-
Notifications
You must be signed in to change notification settings - Fork 21
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added setter to observed properties #57
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you take a look at the merge conflict?
inject-dist/observers.js
Outdated
set: () => { | ||
return original; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If an application attempts to overwrite one of these properties, would this prevent it from working, since it's not actually setting the new value?
Worth testing if this works:
set: () => { | |
return original; | |
set: (new) => { | |
original = new; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The above gives a number of issues (undefined
or null
), while my proposed solution removes any setters which are added through the app's code. So neither is a good solution as yet. I will try investigate the null
/undefined
to understand what's going on .
In the meantime I tried to rewrite it using a proxy
but we are unable to proxy all objects. For example, it is not allowing me to proxy window.navigator
.
I will close this and re-open when I have a working & tested version. I am not confident that the proposed solution works. |
Fixes #54