Skip to content
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

Support more spec-compliant ScriptableObject extensions #1771

Merged
merged 3 commits into from
Dec 30, 2024

Conversation

gbrail
Copy link
Collaborator

@gbrail gbrail commented Dec 23, 2024

This fixes a problem in which ScriptableObject subclasses couldn't support some of the newer code, like Object.assign, because it depends on internal implementation details of ScriptableObject. This changes two things:

  • Adds new overridable methods to ScriptableObject called "putOwnProperty".
  • Call those in internal places instead of calling "putImpl" directly.
  • Catch exceptions in Object.assign if "getAttributes" throws an exception, as the default implementation does.

This makes it possible to address the problem identified in #1558.

To be fully compatible, a ScriptableObject subclass that wants to add its own properties should override "putOwnProperty" INSTEAD of overriding "put" and delegate to the superclass for any unknown properties.

Added a new test that demonstrates this technique.

I'd be interested in feedback here -- this is not a fix to all spec-compliance problems but it's not the ugliest fix in the world either.

Add "putOwnProperty" methods, which can return boolean and be used in
strict mode, and wire them in so that there is no need to call "putImpl"
directly or override it.

Also, allow subclasses that don't override the attributes methods in
ScriptableObject to work as well.
@gbrail gbrail changed the title Support more spec-compliant ScriptableObject exteensions Support more spec-compliant ScriptableObject extensions Dec 23, 2024
@gbrail gbrail added this to the Release 1.8.0 milestone Dec 23, 2024
@rbri
Copy link
Collaborator

rbri commented Dec 24, 2024

ok for me

do you have an idea why this does not fix getOwnPropertyDescriptor/trap-is-null-target-is-proxy.js
while it fixes all the others especially getOwnPropertyDescriptor/trap-is-undefined-target-is-proxy.js ?

@gbrail
Copy link
Collaborator Author

gbrail commented Dec 24, 2024

Something like this might also be a path forward for getting strict mode right -- we can keep Scriptable, but add a new interface for objects that want to extend in a more spec-compliant way, and have code in ScriptableObject and ScriptRuntime adapt if the new interface is present.

@gbrail
Copy link
Collaborator Author

gbrail commented Dec 24, 2024

Also, as for the tests that were fixed -- this might be because NativeProxy doesn't implement "getAttributes". Arguably it should, or we should change the contract between the runtime and ScriptableObject extensions more.

@gbrail gbrail merged commit d243106 into mozilla:master Dec 30, 2024
3 checks passed
@gbrail gbrail deleted the greg-fix-assign branch December 30, 2024 22:51
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