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

Constant folding in development builds #657

Closed
Xiot opened this issue Jan 10, 2021 · 8 comments
Closed

Constant folding in development builds #657

Xiot opened this issue Jan 10, 2021 · 8 comments

Comments

@Xiot
Copy link

Xiot commented Jan 10, 2021

I've noticed a case where the constant folding isn't happening where I thought it should.

It works great in tertirary operators, however it doesn't seem to fold with constant if conditions.

echo "if (process.env.NODE_ENV === 'production') {
  module.exports = require('./cjs/react-dom.production.min.js');
} else {
  module.exports = require('./cjs/react-dom.development.js');
}" | esbuild --define:process.env.NODE_ENV=\"production\"

if (true) {
  module.exports = require("./cjs/react-dom.production.min.js");
} else {
  module.exports = require("./cjs/react-dom.development.js");
}

Looking at #371 it appears like that should be folded properly.

If I enable minification, it does do the proper foldinig.

echo "if (process.env.NODE_ENV === 'production') {
  module.exports = require('./cjs/react-dom.production.min.js');
} else {
  module.exports = require('./cjs/react-dom.development.js');
}" | esbuild --define:process.env.NODE_ENV=\"production\" --minify

module.exports=require("./cjs/react-dom.production.min.js");
@evanw
Copy link
Owner

evanw commented Jan 12, 2021

This is true, but why is this a problem?

@Xiot
Copy link
Author

Xiot commented Jan 12, 2021

For the same reason that you explained in the linked issue. It throws off static analysis and bundling.
It's not a huge issue for me, as --minify-syntax will do the proper folding and I can use that before bundling.

It's just something that I noticed that didn't seem to work the way it "should".

@nettybun
Copy link

I see why you think it should fold, but there's certainly people who wouldn't want it to. Makes it easier to diff code before and after using --define flags in large projects, for instance.

@Xiot
Copy link
Author

Xiot commented Jan 12, 2021

I was going off of what @evanw said in the other thread.
@heyheyhello , do you think this should automatically fold?

echo "const value = flag ? 'prod' : 'dev';" | esbuild --define:flag=false
const value = "dev";

The reason that I brought it up was the incosistency in folding between tertiary operators and if statements

@nettybun
Copy link

Oh interesting, I didn't know about ternary folding. Personally I don't think it should fold. Thanks for the catch. It would be nice if it was consistent, whichever way it goes.

@evanw
Copy link
Owner

evanw commented Jan 12, 2021

This whole feature is motivated by these few lines of code in React, which makes doing this necessary. When not minifying, esbuild still does enough constant folding to be able to determine that process.env.NODE_ENV === 'production' is a constant so it knows which branch is dead. That then tells esbuild that it can ignore the require() call inside the dead branch and avoid the overhead of parsing all of React twice, which is a performance win because the React library is a lot of code. So not doing this constant folding would have a performance cost. But esbuild doesn't go further than that because minification is not technically enabled, and some people have requested that esbuild not remove dead code when not minifying. So the current behavior is intentional and is motivated by practical concerns.

@Xiot
Copy link
Author

Xiot commented Jan 12, 2021

Not removing dead code when not minifying makes a lot of sense, and I agree that the its the way to go.
In this case, should the tertiary operator be folded when minification is disabled ?

@evanw
Copy link
Owner

evanw commented Jan 14, 2021

Sure, I can turn off dead code elimination for that specific case. Previously it was only done for expressions but not for statements. With this change it will now be done for all expressions except no longer for conditional expressions.

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 a pull request may close this issue.

3 participants