Skip to content

Generator added#1

Open
fisgeci wants to merge 4 commits into
masterfrom
proxy-generate
Open

Generator added#1
fisgeci wants to merge 4 commits into
masterfrom
proxy-generate

Conversation

@fisgeci

@fisgeci fisgeci commented Jan 18, 2022

Copy link
Copy Markdown
Owner

No description provided.

Comment thread proxiTest.js Outdated
return hasObject;
}

jsonData = makeLoggable(jsonData, "root");

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a good practice would be to split tests from the service/logic. It would be a good opportunity to learn more about testing on JS (jest would be a good start).

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will do some research on it and try to implement it. Thank you for the suggestion.

Comment thread proxiTest.js Outdated
function makeLoggable(object, parentKey) {
let loggableObj = {};

if (object instanceof Array) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how would this behave when dynamically adding a new attribute (object or array) to one of the users?
i.e
jsonData[0].friends = ["userId1", "userId2"]

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now i seem to have made an error when adding new items of type Object or Array. I did some further testing and it seems that they are not converted into proxies even tho they should. I am working on a fix right now.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now i seem to have made an error when adding new items of type Object or Array. I did some further testing and it seems that they are not converted into proxies even tho they should. I am working on a fix right now.

@fisgeci why would it work?

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now it works cause when i set a new property to the object, the setter just sets the property and does't genereate a proxy for it. In the case you presented it would just add a property with key friends which is an array. If we however attempt to set the value properly, with how the code is currently it will fail. This is because it will try to generate a proxy from a non object value. Which throws an exception(I worked under the assumption that the array only holds objects).

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed the code so now when we try to set a new property it check if that property is of type object. If true it will generate a proxy for it. The result of this approach can be seen below:

https://files.slack.com/files-pri/T042JM9R7-F02U2Q7TCCX/screenshot_2022-01-18_at_17.49.23.png

The friends array now is properly set in the jsonData object and is a proxy so that it can keep track of its properties.

@fisgeci

fisgeci commented Jan 19, 2022

Copy link
Copy Markdown
Owner Author

I believe i have fixed the changes that were suggested and i think its ready for review again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants