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

Fix ReflectShims.defineProperty is undefined in WSH #471

Closed
wants to merge 2 commits into from

Conversation

gnh1201
Copy link
Contributor

@gnh1201 gnh1201 commented Jul 29, 2021

* Reverted from paulmillr#466 - No need to check that this is 'undefined'.
* This pull request fix the following issue: paulmillr#470 paulmillr#467
Comment on lines -3588 to +3585
if (supportsDescriptors) {
if (!ReflectShims.defineProperty) { // possible alternative: if (supportsDescriptors)
Copy link
Collaborator

Choose a reason for hiding this comment

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

supportsDescriptors is only true when get/set are available - but ReflectShims.defineProperty might be available in the same way Object.defineProperty might be - working fine for enumerable/configurable/writable data properties, but throwing for everything else. What does this fix?

Copy link
Contributor Author

@gnh1201 gnh1201 Jul 29, 2021

Choose a reason for hiding this comment

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

This change is a key point to solve #466 and other overrideNative related crash issues.

In WSH(polyfilled by core-js), supportsDescriptors(es6-shim) is false bacause of if(!supportsAccessors && (hasGetter || hasSetter))(es5-shim) is true.

Because of this, replacement parameter is undefined when calling overrideNative. This causes the wrong override.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This part should be run regardless of the values in supportsDescriptors.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm still a bit unclear on your explanation.

If you're in WSH, polyfilled by core-js, why are you running es5-shim and es6-shim at all? You're supposed to use core-js OR es-shims, not both.

es6-shim.js Show resolved Hide resolved
@gnh1201
Copy link
Contributor Author

gnh1201 commented Aug 8, 2021

Any update?

@gnh1201
Copy link
Contributor Author

gnh1201 commented Aug 11, 2021

I'll open it again when the issue becomes clear.

@gnh1201 gnh1201 closed this Aug 11, 2021
@ljharb
Copy link
Collaborator

ljharb commented Aug 11, 2021

I’d love to understand what’s going on - do issues appear when not using core-js?

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.

None yet

2 participants