-
Notifications
You must be signed in to change notification settings - Fork 11
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
fix(Node): Do not drop readonly attributes but only forbid updating them #967
Conversation
Bundle ReportChanges will increase total bundle size by 1.97kB ⬆️
|
See error when upgrading files on server (and we need to backport to 29 and 28!): |
Attributes that are readonly should still be listed in the `attributes` getter, otherwise this is a hard breaking change as a lot of apps already depend on `attributes`. So instead only ensure that readonly attributes are skipped by the proxy handler. Signed-off-by: Ferdinand Thiessen <[email protected]>
13eb986
to
4a7ece4
Compare
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.
I disagree, why do you use the attributes.size
?
That is the issue in the first place, no?
It was never documented to not use attributes for We can branch off if you like and spin up a new major for the removed attributes, but I think in general it makes more sense to keep the attributes and just make them readonly (like I did here). |
f1142dc
to
f11ad8c
Compare
…gh Node.attributes Signed-off-by: Ferdinand Thiessen <[email protected]>
f11ad8c
to
12b6dcc
Compare
Added a test case to prevent doing: file.attributes.owner = 'foo' As this is also protected |
for (const [name, value] of Object.entries(attributes)) { | ||
try { | ||
if (value === undefined) { | ||
delete this.attributes[name] | ||
} else { | ||
this.attributes[name] = value | ||
} | ||
} catch (e) { | ||
// Ignore readonly attributes | ||
if (e instanceof TypeError) { | ||
continue | ||
} | ||
// Throw all other exceptions | ||
throw e |
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.
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.
Also, you can't remove attributes anymore (unless you know they exist on the node already)
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.
Attributes that are readonly should still be listed in the
attributes
getter, otherwise this is a hard breaking change as a lot of apps already depend onattributes
. So instead only ensure that readonly attributes are skipped by the proxy handler.