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

transform-runtime transpiles non-CallExpressions of Object.assign #10859

Open
eps1lon opened this issue Dec 11, 2019 · 10 comments
Open

transform-runtime transpiles non-CallExpressions of Object.assign #10859

eps1lon opened this issue Dec 11, 2019 · 10 comments

Comments

@eps1lon
Copy link
Contributor

@eps1lon eps1lon commented Dec 11, 2019

Bug Report

Current Behavior
Object.assign will be transpiled by transform-runtime

Input Code

Object.assign

Expected behavior/code
A read of a function should not cause transpilation since this might when running feature detection. It should match how transform-object-assign behaves.

Babel Configuration (babel.config.js, .babelrc, package.json#babel, cli command, .eslintrc)
See test file

Environment
babel repo

Possible Solution
Provide transform-runtime with information which static properties are functions. transform-object-assign isn't transpiling non-CallExpressions either.

Additional Context
mui-org/material-ui@0de54dd#r36378503

@babel-bot

This comment has been minimized.

Copy link
Collaborator

@babel-bot babel-bot commented Dec 11, 2019

Hey @eps1lon! We really appreciate you taking the time to report an issue. The collaborators on this project attempt to help as many people as possible, but we're a limited number of volunteers, so it's possible this won't be addressed swiftly.

If you need any help, or just have general Babel or JavaScript questions, we have a vibrant Slack community that typically always has someone willing to help. You can sign-up here for an invite."

@zloirock

This comment has been minimized.

Copy link
Member

@zloirock zloirock commented Dec 11, 2019

var assign = Object.assign is also non-CallExpression. If you use it as a part of an expression like by your link - it should be transpiled since it's a part of features detection and it's available. If you write something like in your example above - it's just a problem of your code.

@nicolo-ribaudo

This comment has been minimized.

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo commented Dec 11, 2019

Yes, Babel should simulate a native environment as much as possible. It can (and does) do much more than simply replacing function calls.

@eps1lon

This comment has been minimized.

Copy link
Contributor Author

@eps1lon eps1lon commented Dec 12, 2019

var assign = Object.assign is also non-CallExpression

I'm not sure how you got the impression that I think otherwise?

If you write something like in your example above - it's just a problem of your code.

if (Object.assign === undefined) is a problem of my code, why?

Yes, Babel should simulate a native environment as much as possible.

Just to clarify: Would you agree with me that the current behavior is a problem or not?

it should be transpiled since it's a part of features detection and it's available

These statements contradict each other. If I'm doing feature detection it shouldn't be transpiled. Otherwise the feature detection is useless: If if (Object.assign === undefined) is transpiled then it will be equivalent to if (false) and is subject to dead code elimination.

Could any of you adress the behavior mismatch between transform-runtime and transform-object-assign?

@zloirock

This comment has been minimized.

Copy link
Member

@zloirock zloirock commented Dec 12, 2019

A simple example:

if (!Object.assign) throw TypeError('Please, use engines with `Object.assign` support');
@eps1lon

This comment has been minimized.

Copy link
Contributor Author

@eps1lon eps1lon commented Dec 12, 2019

A simple example:

For what?

We don't want to throw in the block. Just skip using e.g. logging libraries that need Object.assign.

@zloirock

This comment has been minimized.

Copy link
Member

@zloirock zloirock commented Dec 12, 2019

The example that if it will not be transpiled users will have an error instead of returning work like in engines with native Object.assign support.

@eps1lon

This comment has been minimized.

Copy link
Contributor Author

@eps1lon eps1lon commented Dec 12, 2019

If that is a concern then don't change it. But you commented on the linked PR introducing even more transpilation. At least leave the status quo so that the behavior stays the same.

Third times the charm: Why does transform-object-assign not work this way?

@eps1lon

This comment has been minimized.

Copy link
Contributor Author

@eps1lon eps1lon commented Dec 12, 2019

Also

if (!Object.assign) throw TypeError('Please, use engines with Object.assign support');

This is still subject to dead code elimination. Why would you use this code and the transform?

@zloirock

This comment has been minimized.

Copy link
Member

@zloirock zloirock commented Dec 12, 2019

transform-object-assign is not my work, so I have no ideas.

This is still subject to dead code elimination.

Yes, it's the case for dead code elimination and I think that this transform should be improved in the future. But cases could be more complex and dead code elimination will not work.

zloirock referenced this issue in mui-org/material-ui Dec 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.