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

Use es2015-loose Babel preset in production build #198

Closed
taion opened this issue Jul 26, 2016 · 37 comments
Closed

Use es2015-loose Babel preset in production build #198

taion opened this issue Jul 26, 2016 · 37 comments

Comments

@taion
Copy link

taion commented Jul 26, 2016

Almost every React package that I'm aware of uses the loose version of the preset to transpile, given that es2015-loose produces code that's easier to understand and more performant.

As such, it would probably make the most sense to use es2015-loose at least for the production Babel config. There are a few edge cases where it allows illegal code to work, so it may not be appropriate for the development config, but I think any "serious" production Babel config should be expected to use es2015-loose rather than es2015.

@taion
Copy link
Author

taion commented Jul 26, 2016

Lifted from #189 (comment):

Loose mode generates code that is simpler, but less strictly compliant to spec. Compare output:

I'm not aware of any reason to not at least run your production build in loose mode. I'm also not aware of anybody who deliberately uses non-loose mode.

@lacker
Copy link
Contributor

lacker commented Jul 26, 2016

Disclaimer: everything I know about loose mode I learned from this blog post: http://www.2ality.com/2015/12/babel6-loose-mode.html

That said, from that link:

Normally, it is recommended to not use loose mode. The pros and cons are:

Pros: The generated code is potentially faster and more compatible with older engines. It also tends to be cleaner, more “ES5-style”.
Con: You risk getting problems later on, when you switch from transpiled ES6 to native ES6. That is rarely a risk worth taking.

Do you disagree?

Also, it appears from npm stats that regular mode is about 20x as popular as loose mode. So es2015-loose is at least not the most popular thing in the community.

@taion
Copy link
Author

taion commented Jul 26, 2016

@lacker

These are the most prominent examples I can think of from around the React ecosystem:

Redux: https://github.com/reactjs/redux/blob/v3.5.2/.babelrc#L3-L18
React Router: https://github.com/reactjs/react-router/blob/v3.0.0-alpha.2/.babelrc#L7-L11
React-Bootstrap: https://github.com/react-bootstrap/react-bootstrap/blob/v0.30.0/.babelrc#L4 (I know it's still on Babel 5... sigh)

React: https://github.com/facebook/react/blob/v15.2.1/.babelrc#L11-L19
babel-config-fbjs: https://github.com/facebook/fbjs/blob/eslint-config-fbjs-v1.0.0/packages/babel-preset-fbjs/configure.js#L60-L68

I'm not exaggerating when I say that I'm not aware of any packages that are intentionally not using loose mode.

There might be an argument for e.g. running test suites against both loose mode and strict mode, but I'm not aware of anybody who actually does so.

As far as I'm aware, almost all "real" projects just use es2015-loose, when their authors are aware of that option.

@vjeux
Copy link
Contributor

vjeux commented Jul 26, 2016

We use loose mode inside of fb on react native

@gaearon
Copy link
Contributor

gaearon commented Jul 26, 2016

I'm good with using loose mode.

@hzoo
Copy link

hzoo commented Jul 26, 2016

Same with babel itself https://github.com/babel/babel/blob/master/package.json#L50

You would think it would be the opposite; that you would want to run more spec compilant 😄, what about add-module-exports then https://github.com/59naga/babel-plugin-add-module-exports?

@gaearon
Copy link
Contributor

gaearon commented Jul 26, 2016

We want people to use import and export over require so I probably wouldn’t go with it.

@vladshcherbin
Copy link

vladshcherbin commented Jul 26, 2016

I've seen only one piece of code, that didn't work the same in loose mode:

Code from SO answer won't give you the same in loose mode

[...Array(10).keys()]
//=> [0, 1, 2, 3, 4, 5, 6, 7, 8, 9]
-
//=> [ArrayIterator] in loose mode

Similar example from - developer.mozilla.org, denseKeys output won't be same in loose mode.

var arr = ["a", , "c"];
var sparseKeys = Object.keys(arr);
var denseKeys = [...arr.keys()];
console.log(sparseKeys); // ['0', '2']
console.log(denseKeys);  // [0, 1, 2]
-
console.log(denseKeys);  // [ArrayIterator] in loose mode

@insin
Copy link
Contributor

insin commented Jul 26, 2016

I'd say babel-plugin-add-module-exports is only necessary for stuff you'll be publishing to npm, so your non-transpiling users don't have to eat a .default.

Most non-official Babel plugins I've tried have needed a .default tacked on, which is a bit of a mess :/

@hzoo
Copy link

hzoo commented Jul 26, 2016

Right this would probably be for an application anyway and everything in here would be import/export like Dan mentioned

@vladshcherbin that's because in loose mode that transpiles to [].concat(Array(10).keys()).

[].keys returns an iterator and non-loose mode does a Array.from() call to turn it into an array

@vladshcherbin
Copy link

@hzoo yes.

There should be a note somewhere that babel in production is in loose mode as this examples will work different in dev / production and especially for a novice it won't be obvious to understand what's wrong.

@gaearon
Copy link
Contributor

gaearon commented Jul 26, 2016

If we enable loose, we enable it everywhere. Having differences between dev and prod is bad.

@ForbesLindesay
Copy link
Contributor

Ideally I think we'd like a loose-with-warnings that behaves exactly the same as loose mode, but console.warns whenever you do something that would behave differently in strict mode.

We cannot have different behaviour in production vs. dev (couldn't agree more on that point) but we can/should warn users in development mode.

@gaearon
Copy link
Contributor

gaearon commented Jul 26, 2016

Are there any lint rules against things that loose does differently?

@ForbesLindesay
Copy link
Contributor

I think mostly they can't be detected statically (in the general case) otherwise we could just have one babel config that would do the loose mode thing when it made no difference to behaviour and the strict thing when it mattered.

At runtime though, you could detect that something is iterable but not an array for example.

@taion
Copy link
Author

taion commented Jul 26, 2016

I had an idea that it'd be cool to run tests against both es2015-loose and es2015.

I've never gone through with that idea and I haven't seen anyone do anything like it.

But it's worth bringing up. Users are already eating the downsides of loose mode because all their dependencies will be using loose mode. Might as well let them have the benefits in their code too.

@ForbesLindesay
Copy link
Contributor

My biggest concern here is that we leave apps in a state where they can't disable the babel transform even once all browsers support the features natively. We need to do what we can to discourage people relying on it being loose.

I do recognise that the performance benefits of loose are to big to ignore though.

@kasperpeulen
Copy link
Contributor

I'm wondering, about performance benefits. This may be a very stupid comment, but as Chrome, Firefox, Safari 10 and Edge all can run ES2015 code, it sounds as a waste to me to not make use of that.

For the production config, wouldn't it be more performant, to have a small script in the beginning that determines the browser and the version of the browser, and runs an ES5 bundle if it is IE or a mobile browser (or an old version of chrome,firefox,safari), and otherwise runs an native es2015 bundle?

Just an idea, I may say something really stupid here.

@taion
Copy link
Author

taion commented Jul 27, 2016

@kasperpeulen

That's sort of the dream, but it's hard to get to. You'd need to make multiple builds, feature-sniff appropriately for a wide set of features on the client, then ship the appropriate build.

But it's actually a bit worse than that, because if you really want to ship ES2015 code, you want your dependencies to expose an ES2015 build, and then figure out how to use that for your "real ES2015" build (as contrasted with module and jsnext:main which still mostly transpiles to ES5 but uses ES2015 modules).

It's the right thing to do, but the degree of complexity is quite high, and I'm not aware of any good tooling around even doing the more limited goal of shipping proper ES2015 code for your own application (otherwise I'd be using it).

@omrigilad
Copy link

I have to say, humbly, that I'm completely baffled by this thread. You've clearly shown that some incredible projects, developed by really talented people, use loose mode.

I'm still trying to wrap my head around why you would think that non-standard behaviour is what's best for the average user.

Which do you think the average user would prefer?

  • use ES6 today. ES6 is awesome!
    or
  • use ES6 today. ES6 is awesome! Oh, except Array.prototype.keys, that returns an ArrayIterator and that doesn't work. What's an ArrayIterator and why should you care? Read this GitHub comment and hope to understand and find out!

The way I see it, perhaps the very talented people who work on React or Babel or other impressive projects that use loose mode, are smart enough to still be productive while working with the mental constraint of having to remember what's an ArrayIterator and what isn't and where they may use the spread operator as-is and where they may not. I'd like to argue that for the average user this is a significant constraint that should be avoided.

I sincerely hope I'm not coming off as too aggressive, but there are some very smart people weighing in on this, and I find myself completely disagreeing with you. I'd really like to understand where I'm wrong, if I'm wrong.

@gaearon
Copy link
Contributor

gaearon commented Jul 28, 2016

I think the difference is probably negligible for the use cases of most apps, and we should err on the side of no surprises. (Correct me if I’m wrong.) Libraries are a different story because they usually do a lot of work and need to be performant for everyone. So “we’d like consistent ES6 features” is probably less useful for libraries (whose authors can constrain themselves), but I’m not sure we should enable loose for apps, especially considering this tool is meant for beginners as well.

@hzoo
Copy link

hzoo commented Jul 28, 2016

@mmgm No, I would agree as well - it is true that in the majority of cases it won't matter but most users don't know what loose mode provides/does and we don't want users relying on non-spec behavior just to make the output a bit shorter/faster.

@taion
Copy link
Author

taion commented Jul 28, 2016

Worth noting that as-is, es2015 produces code that can't run in ES5 environments without polyfills – https://phabricator.babeljs.io/T7518

es2015-loose doesn't have this problem.

@gaearon
Copy link
Contributor

gaearon commented Jul 28, 2016

Woah, this sure is going to be confusing to Redux users (who use [...] a lot).
Unless there is a simple fix for this on Babel side, I’d flip to loose then.

@omrigilad
Copy link

omrigilad commented Jul 28, 2016

I think @taion's comment is a bit misleading.

If we reach the code path where Array.from is called, then a is not an array and [...a] is guaranteed to be an [a] in loose mode, ie. to not work, but not throw an error.

I agree that this is seemingly a bug in Babel (unless it's their strategy to not polyfill functionality but only transform syntax. Is that the case?), but it's certainly not resolved by switching to loose.

@taion
Copy link
Author

taion commented Jul 28, 2016

To be specific – the problem there is that if you try to spread a non-array value, es2015 will try to call Array.from. If you have a polyfill, then it will work.

If you don't have a polyfill:

  • If your browser supports Array.from, then you will get the correct behavior
  • If your browser does not support Array.from, then you will get a runtime error

es2015-loose will do the wrong thing, but it will consistently do the wrong thing, with or without a polyfill, in every browser.

This is less an argument for es2015-loose than to point out that es2015, with the current setup (i.e. no broad ES2015 polyfills), can also lead to very surprising behavior.

@taion
Copy link
Author

taion commented Jul 28, 2016

@mmgm Yeah you're absolutely right. This is much more of an argument to either polyfill the ES2015 functionality that the helpers need, or for Babel to fix this bug upstream. es2015-loose only avoids this problem by doing the wrong thing, which is something that has actually bitten me before.

@gaearon
Copy link
Contributor

gaearon commented Jul 28, 2016

I think that for now, we’ll leave es2015 as is, and see how it goes.
If we get many reports about confusing behavior or bad performance, we can revisit this.
Thanks!

@gaearon gaearon closed this as completed Jul 28, 2016
@hzoo
Copy link

hzoo commented Jul 28, 2016

@mmgm Right, Babel as a transform won't polyfill anything by itself since it only does syntax. For that we have babel-polyfill or babel-plugin-transform-runtime. http://babeljs.io/docs/usage/caveats/#polyfills

@taion
Copy link
Author

taion commented Jul 28, 2016

@hzoo

Is it intentional that Babel transforms syntax into something that requires a polyfill, though?

It's unintuitive to me that transpiling [...'foo'] requires a polyfill. The person writing that is only using syntax directly.

@taion
Copy link
Author

taion commented Jul 28, 2016

The specific danger here is that, if I do e.g. [...'foo'] and don't ship a polyfill – my code will work just fine in the recent version of Chrome that I myself use, but it will break for my users on browsers without ES2015 environments.

@loganfsmyth
Copy link

loganfsmyth commented Jul 28, 2016

@taion I dislike Array.from because it means this logic works with general array-like objects, which is not spec compliant, but it's not clear that a change to fix that would address your concerns. No matter what, spread is defined in terms of Symbol, so you'd going to get this problem no matter what.

The same limitation applies for for...of. Babel does indeed assume you are loading a polyfill and has always done so.

@taion
Copy link
Author

taion commented Jul 28, 2016

@loganfsmyth Got it. My misunderstanding, thanks.

@taion
Copy link
Author

taion commented Jul 28, 2016

I wish there were a good way to just throw there, though. There's nothing enforcing that the correct polyfills get pulled in, and inadvertently ending up with code that works for me but not my users is a really bad feeling.

@loganfsmyth
Copy link

Yeah I don't disagree there, Babel doesn't currently do a great job of signaling any of that.

@omrigilad
Copy link

Also worth noting that, if I understand correctly, generators are transpiled to code that not only requires a polyfill, but requires a specific runtime library to essentially pollute your global scope (unless you use plugin-transform-runtime).

I understand your issue with this approach and for what it's worth, I completely agree with you. But this seems like more of Babel-wide problem than an issue with loose mode or regular (strict? Spec?) mode.

@taion
Copy link
Author

taion commented Jul 29, 2016

I think it's relatively well-known among Babel users that generators require a runtime. I was very surprised to realize that those other language features required polyfills, though – I'd assumed they hadn't.

I opened #269 to note that create-react-app should ship those polyfills; otherwise those transpiled language features will fail for users of applications built with create-react-app that have the misfortune of using old browsers.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests