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

CallableInstance throws error in older ECMA 5 targets #245

Closed
4 tasks done
justinbhopper opened this issue Jun 5, 2024 · 6 comments · Fixed by #246
Closed
4 tasks done

CallableInstance throws error in older ECMA 5 targets #245

justinbhopper opened this issue Jun 5, 2024 · 6 comments · Fixed by #246
Labels
💪 phase/solved Post is done

Comments

@justinbhopper
Copy link
Contributor

Initial checklist

Affected packages and versions

11.0.4

Link to runnable example

No response

Steps to reproduce

The CallableInstance class is using Object.defineProperty without checking descriptor.configurable. When we used unified in a android mobile environment, we saw some differences in how the Processor class was being constructed that caused an error.

The difference we saw was if the javascript is transformed to target ES5 (ECMAScript 5.0), the Processor's copy method had a different number of property names defined than in ES6. Some of these properties are configurable: false, so calling Object.defineProperty on them throws a "Cannot redefine property: arguments" error.

In ES5:
image

In ES6 (Chrome):
image

Notice in ES6, the number of property names is simply ['length', 'name'], but in ES5 it includes additional property names that are not configurable. I believe simply checking configurable would solve this issue.

Expected behavior

Should be able to call unified() without error.

Actual behavior

Calling unified() throws an error "Cannot redefine property: arguments" during the super('copy') within the Processor's constructor.

Affected runtime and version

node@20.11.1

Affected package manager and version

npm@11.0.4

Affected OS and version

No response

Build and bundle tools

No response

@github-actions github-actions bot added 👋 phase/new Post is being triaged automatically 🤞 phase/open Post is being triaged manually and removed 👋 phase/new Post is being triaged automatically labels Jun 5, 2024
@wooorm
Copy link
Member

wooorm commented Jun 5, 2024

I don't think we treat es5 as a support target -- why should this change happen here, is it not a bug in the tools you use to go from modern code to ancient code?

@justinbhopper
Copy link
Contributor Author

I don't think we treat es5 as a support target -- why should this change happen here, is it not a bug in the tools you use to go from modern code to ancient code?

Even in ES6, checking configurable is proper to guard against this kind of error. I only mentioned ES5 because it was particular to how we encountered the issue, but it's entirely plausible for this error to happen in other circumstances.

Are you opposed to the guarding condition for some reason?

@wooorm
Copy link
Member

wooorm commented Jun 5, 2024

The descriptor is passed to that function, shouldn't it handle its options and not fail?

Extra reason, is that's it's external code which was fine for years so wondering why it's a thing now https://github.com/CGamesPlay/node-callable-instance

@justinbhopper
Copy link
Contributor Author

justinbhopper commented Jun 5, 2024

node-callable-instance would have the same bug as this does.

Object.defineProperty can't defy the rules of the property. The descriptor is passed in because the code is basically saying "define this property and use the same descriptions from this descriptor". However, for properties that aren't configurable, you can't redefine them. They're locked in place.

Since ES5 doesn't support classes, a ES6 class will be transpiled to a traditional function with some helper methods to simulate a object oriented class structure. Traditional functions has a few additional properties that ES6 classes don't have and they cannot be redefined.

image

Here is a playground that might illustrate this:
https://playcode.io/1896334

@wooorm
Copy link
Member

wooorm commented Jun 6, 2024

I guess unified is quite popular so that might be why it pops up here, now.

Object.defineProperty

👍

This comment has been minimized.

@wooorm wooorm added the 💪 phase/solved Post is done label Jun 19, 2024
@github-actions github-actions bot removed the 🤞 phase/open Post is being triaged manually label Jun 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💪 phase/solved Post is done
Development

Successfully merging a pull request may close this issue.

2 participants