Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Target Babel to latest Chrome Versions in dev #3806

Merged
merged 2 commits into from Dec 11, 2016
Merged

Conversation

ngotchac
Copy link
Contributor

Use babel-preset-env to target Babel to latest Chrome Versions.

This increases re-build speeds by 25% (from ~4s to ~3s). It isn't much yet, but could eventually get better while Chrome is shipping new features.

@ngotchac ngotchac added A0-pleasereview 🤓 Pull request needs code review. M7-ui labels Dec 11, 2016
@coveralls
Copy link

Coverage Status

Coverage increased (+0.003%) to 85.616% when pulling 0310a02 on babel-preset-env into 068201a on master.

@jacogr
Copy link
Contributor

jacogr commented Dec 11, 2016

I'm a bit concerned doing this - the amount of times I have to swap to FF and/or Safari (and really should be IE as well) to test the platforms and address issues is a lot. I fear that this may make it more difficult to have all-over compatibility. (Since the dev tools is important when looking at other browsers)

@ngotchac
Copy link
Contributor Author

ngotchac commented Dec 11, 2016

Ok fair. So what about these 3 options :

  1. Opt-out mechanism by setting BABEL_PRESET_ENV=false not to run these optimizations
  2. Specify last 2 versions instead of last 2 Chrome versions which would target last 2 versions of every browsers plus 1. opt-out mechanism
  3. Opt-in mechanism where only setting BABEL_PRESET_ENV=true would run these optimizations

I would rather have the 1. option since I am using Chrome most of the time, and having Safari support in dev mode right now means not using any ES2016 features. I could also go with last 2 Chrome versions, last 2 Firefox versions to target Chrome and Firefox by default.

@jacogr
Copy link
Contributor

jacogr commented Dec 11, 2016

We need to make is easier to swap browsers, not more difficult. I would not want to jump through hoops to test and run.

(As an aside, actually thinking of swapping my default way from Chrome just to ensure we are getting some better coverage cross browser, each time I try in others there are obvious nigglies that pop up, rather catch it earlier)

@jacogr
Copy link
Contributor

jacogr commented Dec 11, 2016

PS: I also prefer a dev build that is closer to the production build instead of further away - I'd rather not have too many repeats of the Transfer issues out there which we couldn't replicate. (I'd rather have to deal with one thing only, Uglify, than more code-generation that is not in-sync)

@ngotchac
Copy link
Contributor Author

Okay, I'll change to option 3. then with opt-in. Still think that most users running npm start won't use Safari or IE

@ngotchac
Copy link
Contributor Author

Well the dev environment won't be the same as the build one anyway ; we don't want to run UglifyJS and stuff while developing. That's why there is:
DAPPS_URL="/" PARITY_URL="127.0.0.1:8180" NODE_ENV="production" npm run build && node webpack/build.server which can be used to replicate a "production" env

@jacogr
Copy link
Contributor

jacogr commented Dec 11, 2016

The point is that with that setup it really doesn't really get used that at all, expect maybe by you in some cases - just another option that doesn't get full exposure and just seems complicated. (I wouldn't even know how to start it, let alone remember all the required options.)

Much easier to actually just build a full Parity, since it is "standard" and actually reflects as-run - even though it takes ages.

And yes, I mentioned Uglify - already have that to deal with, more moving parts is a no-go by default.

@ngotchac
Copy link
Contributor Author

ngotchac commented Dec 11, 2016

Well it would be as simple as running BABEL_PRESET_ENV="true" npm start instead of npm start.

The thing is that the dev environment isn't a test environment, but really something to make development easier and faster. There are already those differences (in dev mode):

  • Webpack is caching transpiled files
  • Source-Map is handled in a different way
  • Babel-Loader is caching transpiled files
  • A Common chunk (to the UI and all dApps) is used so we have faster rebuilts
  • UglifyJS is not being used
  • Webpack OccurrenceOrderPlugin plugin is used differently
  • CSS files are not extract but loaded from the JS

The point is : we want to make developing the UI easier and not necessarily reproducing exactly the production environment at each step of developing a new feature. I think that before merging a PR, we should use this build.server script and related command to see if everything is fine

@coveralls
Copy link

Coverage Status

Coverage remained the same at 85.613% when pulling eb1c628 on babel-preset-env into 068201a on master.

@jacogr
Copy link
Contributor

jacogr commented Dec 11, 2016

Happy with opt-in as it is.

The point is though - if the dev environment doesn't replicate compiled in terms of functionality & browser compatibility, it doesn't matter how fast it is, it is lacking. (Things like extracting CSS, dupes & commons doesn't change behaviour)

I don' think running another difficult to configure environment (as it stands) is a solution since it is an extra step since it will get missed and as it stands we are not getting through everything fast enough. If that is the solution, we should just develop against it. (Which is not possible and very unproductive.)

@jacogr jacogr added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Dec 11, 2016
@jacogr jacogr merged commit d3077c5 into master Dec 11, 2016
@jacogr jacogr deleted the babel-preset-env branch December 11, 2016 14:05
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants