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

Avoid __proto__ syntax #3140

Closed
GrosSacASac opened this issue Sep 30, 2019 · 3 comments
Closed

Avoid __proto__ syntax #3140

GrosSacASac opened this issue Sep 30, 2019 · 3 comments

Comments

@GrosSacASac
Copy link

Expected Behavior / Situation

rollup does not use deprecated syntax.

Actual Behavior / Situation

rollup uses the deprecated __proto__ syntax https://www.ecma-international.org/ecma-262/10.0/index.html#sec-additional-ecmascript-features-for-web-browsers

Just saw #3136

Modification Proposal

Replace

var x = Object.freeze({
  __proto__: null,
  bar: bar
});

with

var x = Object.freeze(Object.assign(Object.create(null), {
  bar: bar
}));

or

var x = {
  bar: bar
};
Object.setPrototypeOf(x, null);
Object.freeze(x);

or

var x = Object.freeze(Object.setPrototypeOf({
  bar: bar
}, null));
@mheiber
Copy link

mheiber commented Oct 1, 2019

What are the trade-offs between the approaches?

  • Approach 1 (current): __proto__
  • Approach 2: Object.create()
  • Approach 3: Object.setPrototypeOf then freeze in a separate line
  • Approach 4: Object.setPrototypeOf and freeze in a single line

@GrosSacASac
Copy link
Author

Approach 1 is recommended against in the specification.
Approach 2 and 3 might have performance differences.
Approach 3 and 4 is essentially the same, with different coding styles.
I would initially go with approach 3. And optimize later.

Do we want to change this ?

If yes I could submit a PR, otherwise feel free to close the issue.

@lukastaegert
Copy link
Member

I cannot find any specific recommendation in the document you linked. As a matter of fact, this document properly defines the semantics of accessing __proto__, the only caveat being that it is an optional feature. However to my knowledge, all commonly used JS runtimes support it. The reasons why it should not be used are the same why Object.setPrototypeOf should not be used–mutating or switching the prototype will deoptimize the runtime. This unfortunately rules out approaches 3 and 4. The advantage of the approach we chose is that the prototype is changed before any optimization took place. We even consulted a Google engineer about possible performance concerns: #3096 (comment)

Also it is very concise syntactically. The Object.assign approach itself has bad performance that depends on the number of copied properties and also does unnecessary shape mutations which are slow themselves.

Thus our approach is the only one I would feel good to have enabled by default, and It has already been discussed with various experts. If there are concerns about the optional nature of the feature, we would be open to a PR that adds an option to deactivate it, similar to output.freeze.

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

No branches or pull requests

3 participants