Skip to content
This repository has been archived by the owner on Feb 18, 2024. It is now read-only.

Update to Babel 7 #845

Merged
merged 2 commits into from
May 7, 2018
Merged

Update to Babel 7 #845

merged 2 commits into from
May 7, 2018

Conversation

edmorley
Copy link
Member

@edmorley edmorley commented May 3, 2018

Notable changes:

  • All official packages have moved under the @babel/ namespace and in some cases further renamed, so package names adjusted accordingly.
  • @babel/preset-env now includes @babel/plugin-transform-spread and @babel/plugin-transform-classes, which are the Babel 7 renames of babel-plugin-transform-object-rest-spread and babel-plugin-transform-es2015-classes.
  • @babel/preset-env's useBuiltIns: true mode has been renamed to useBuiltIns: 'entry' - which is equivalent.
  • @babel/preset-react now has development and useBuiltIns options, which we set appropriately:
    https://github.com/babel/babel/tree/v7.0.0-beta.46/packages/babel-preset-react#options
  • babel-plugin-dynamic-import-node is no longer required by @neutrinojs/library for target node, since webpack converts the dynamic import to a require() itself.
  • @neutrinojs/jest required more substantial changes since:
  • Several packages now have a peer dependency on @babel/core, so it's been added where necessary.
  • babel-loader has been updated to v8 for Babel 7 compatibility.
  • @neutrinojs/vue's babel-preset-vue dependency doesn't appear to be used, but has been left as-is pending Vue preset's babel-preset-vue dependency is unused #836.

Closes #316.

@edmorley edmorley added this to the v9 milestone May 3, 2018
@edmorley edmorley self-assigned this May 3, 2018
@eliperelman eliperelman self-requested a review May 3, 2018 18:02
@edmorley
Copy link
Member Author

edmorley commented May 3, 2018

I'd be curious to know how both the dev start initial compile and the prod build times of this compared to master, for taskcluster-{web,tools}.

Copy link
Member

@eliperelman eliperelman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything looks good to me, just a question.

// Babel 7 returns null if the file was ignored.
return transform(
src,
Object.assign({}, { filename }, config.globals.BABEL_OPTIONS)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We dropped support for Node.js v6, right? Does that mean we can use rest/spread here instead of assign?

@jaridmargolin
Copy link
Contributor

Hey @eliperelman @edmorley - Just wanted to thank you two for doing an amazing job maintaining this project in an ecosystem that is constantly changing. This as well as the recent webpack 4 updates are why I'm confident I can keep neutrino in my stack.

Thank you 🙏!

@edmorley
Copy link
Member Author

edmorley commented May 4, 2018

You're most welcome! The thanks are very much appreciated ❤️

@eliperelman
Copy link
Member

Thank you so much! We've got some interesting things planned, hopefully I'll get a PR out today.

// https://github.com/facebook/jest/blob/v22.4.2/packages/babel-jest/src/index.js#L105-L147
// And is required due to:
// https://github.com/facebook/jest/issues/1468
// TODO: See if it would be easier to switch to the higher-level babel-jest,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should be able to. If not, please open up an issue 🙂

If you wanna hold off on that, you should make sure to return a sourcemap here, Jest handles them nowadays (should allow you to drop retainLines)

Copy link
Member Author

@edmorley edmorley May 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @SimenB - thank you for taking a look :-)

Yeah there's lots of potential for clean-up in our current Jest config!

The problem I was finding was that mentioned in jestjs/jest#1468, in that the only way to pass the babel options would be to call babel-jest's createTransformer() in our own process() (since that's the earliest we have access to config.globals), which seems a little clunky - plus means we can't use babel-jest's getCacheKey() implementation and still have to duplicate canInstrument: true etc.

eg:

// transformer.js
const { createTransformer } = require('babel-jest');

module.exports = {
  canInstrument: true,
  process(src, filename, config, transformOptions) {
    return createTransformer(config.globals.BABEL_OPTIONS).process(src, filename, config, transformOptions);
};

Though perhaps a better way would be to circumvent config.globals and pass our babel config via process.env like this?

// transformer.js
const { createTransformer } = require('babel-jest');
const babelConfig = JSON.parse(process.env.BABEL_OPTIONS);

module.exports = createTransformer(babelConfig);

Or is there a simpler way I'm missing? (Having to use a custom transformer seems pretty heavyweight for just wanting to not use a .babelrc, but I realise we're likely an edge-case :-))

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've filed #851 for making these changes.

Notable changes:
* All official packages have moved under the `@babel/` namespace and in
  some cases further renamed, so package names adjusted accordingly.
* `@babel/preset-env` now includes `@babel/plugin-transform-spread` and
  `@babel/plugin-transform-classes`, which are the Babel 7 renames of
  `babel-plugin-transform-object-rest-spread` and
  `babel-plugin-transform-es2015-classes`.
* `@babel/preset-env`'s `useBuiltIns: true` mode has been renamed to
  `useBuiltIns: 'entry'` - which is equivalent.
* `@babel/preset-react` now has `development` and `useBuiltIns` options,
  which we set appropriately:
  https://github.com/babel/babel/tree/v7.0.0-beta.46/packages/babel-preset-react#options
* `babel-plugin-dynamic-import-node` is no longer required by
  `@neutrinojs/library` for target `node`, since webpack converts the
  dynamic import to a require itself.
* `@neutrinojs/jest` required more substantial changes since:
  - the custom transformer used the now removed `canCompile()` from
    Babel's API.
  - `babel-preset-jest` is not fully compatible with Babel 7
    (jestjs/jest#6126).
* Several packages now have a peer dependency on `@babel/core`, so it's
  been added where necessary.
* `babel-loader` has been updated to v8 for Babel 7 compatibility.
* `@neutrinojs/vue`'s `babel-preset-vue` dependency doesn't appear to
  be used, but has been left as is pending #836.

Closes #316.
@eliperelman
Copy link
Member

@edmorley this still looks good, anything outstanding before this gets merged?

@edmorley
Copy link
Member Author

edmorley commented May 7, 2018

@eliperelman I think this is fine to merge now - there's a lot of cleanup we can do on the Jest side, but it's mostly pre-existing and I'd like to keep the scope as small as possible here.

@edmorley edmorley merged commit e79fa15 into master May 7, 2018
@edmorley edmorley deleted the babel-7 branch May 7, 2018 12:40
@edmorley edmorley mentioned this pull request May 7, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

Successfully merging this pull request may close these issues.

4 participants