-
-
Notifications
You must be signed in to change notification settings - Fork 30
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
Implement [LegacyLenientSetter]
and test [LegacyLenientThis]
#209
Implement [LegacyLenientSetter]
and test [LegacyLenientThis]
#209
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.
Great stuff, thank you!
brandCheck = ""; | ||
const replaceable = utils.getExtAttr(this.idl.extAttrs, "Replaceable"); | ||
const legacyLenientSetter = utils.getExtAttr(this.idl.extAttrs, "LegacyLenientSetter"); | ||
const legacyLenientThis = utils.getExtAttr(this.idl.extAttrs, "LenientThis"); |
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.
As a bonus, can you enforce (via compile-time errors) the following:
An attribute with the [Replaceable] extended attribute must not also be declared with the [LegacyLenientSetter] or [PutForwards] extended attributes.
The [LegacyLenientSetter] extended attribute must take no arguments. It must not be used on anything other than a read only regular attribute.
An attribute with the [LegacyLenientSetter] extended attribute must not also be declared with the [PutForwards] or [Replaceable] extended attributes.
The [LegacyLenientThis] extended attribute must take no arguments. It must not be used on a static attribute.
The [Replaceable] extended attribute must take no arguments.
An attribute with the [Replaceable] extended attribute must not also be declared with the [LegacyLenientSetter] or [PutForwards] extended attributes.
The [Replaceable] extended attribute must not be used on an attribute that is not read only.
The [Replaceable] extended attribute must not be used on a static attribute.
(I've omitted the not-on-a-namespace checks since we don't do namespaces.)
Enforcing these would make it more obvious why we don't have tests for some of these cases.
If you would rather do these in a separate PR, perhaps including checks for all the other extended attributes we implement, that is fine too. Perhaps at that time we could even introduce a way of testing these errors.
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 intend to do that in its own PR.
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.
LGTM. Will merge alongside #206, likely this weekend.
Closing since this is from an organization and we don't accept PRs from organizations anymore. I'll reopen a new PR instead. |
This also fixes the bug where
[LegacyLenientThis]
would still result in attempting to get the implementation value, even whenexports.is(esValue)
wasfalse
, which would result in aTypeError
ifesValue
did not have animplSymbol
property.This will have merge conflicts with #206 and should probably be rebased on top of it, since it doesn’t make sense to have
[LegacyLenientThis]
use the new naming scheme, but have[LegacyLenientSetter]
using the old naming scheme.Part of #207