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

πŸ—βœ¨ Upgrade to Babel 7 #18574

Merged
merged 15 commits into from
Oct 10, 2018
Merged

πŸ—βœ¨ Upgrade to Babel 7 #18574

merged 15 commits into from
Oct 10, 2018

Conversation

rsimha
Copy link
Contributor

@rsimha rsimha commented Oct 5, 2018

It's been a year since we upgraded to Babel 6 in #9775. It's time to upgrade to Babel 7.

Things this PR does:

  1. Replaces babel-core (version 6) with @babel/core (version 7, from the new babel monorepo)
  2. Updates all other babel packages with their latest monorepo equivalents
  3. Updates babelify from version 8 (compatible with babel 6) to version 10 (compatible with babel 7)
  4. Moves configurations from .babelrc to babel.config.js
  5. Consolidates all the different babelify settings across the codebase into babel.config.js (Fixes Centralize browserify configΒ #18582)
  6. Consolidates build-system/tasks/compile-{access|bind|css}-expr.js into a single file compile-expr.js (and removes the _token-stack: label from the generated jison parser in order to satisfy babel 7)
  7. Removes the global: true setting for babelify added in Upgrade Closure Compiler to v20171112Β #18552 (so that node_modules dependencies are also compiled) and replaces it with a less risky method that does a babelify transform for only those node_modules packages that we ship with the runtime
  8. Eliminates the use of the babel-plugin-transform-remove-strict-mode package that was added during the babel 6 upgrade in upgrade to babel 6Β #9775 to prevent use strict errors, and enables strict mode during development
  9. Fixes instances where code was incorrectly writing to read-only properties, now that strict mode is enabled and enforced during development and unit testing

With this, all traces of babel 6 should be gone from our code-base, and babel 7 should be the only means of compiling unminified code.

Fixes #18654
Fixes #18582
Closes #18199

@rsimha
Copy link
Contributor Author

rsimha commented Oct 5, 2018

@erwinmombay @jridgewell @kristoferbaxter @choumx I'm sending this out for preliminary high level comments while I continue to work on it.

As things stand right now, gulp build and gulp serve work with babel 7. I can get all example pages to load correctly. Still working on some test failures.

@rsimha
Copy link
Contributor Author

rsimha commented Oct 8, 2018

This is now ready for review.

src/log.js Show resolved Hide resolved
babel.config.js Outdated Show resolved Hide resolved
type: 'lalr',
debug: false,
moduleType: 'js',
'token-stack': true,
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm very worried about performance regressions that'll be caused by this. The token-stack appears to create a lot of arrays and array mutations.

Is this still needed? If the _token_stack label still needs to be stripped, can we do it as a post-process step instead of changing the JISON algorithm?

Copy link
Contributor Author

@rsimha rsimha Oct 8, 2018

Choose a reason for hiding this comment

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

Compiling with babel 7 fails without this. I found this fix in the jison codebase. Do you have a suggestion for a better way to strip this label?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jridgewell WDYT?

extensions/amp-gwd-animation/0.1/amp-gwd-animation-impl.js Outdated Show resolved Hide resolved
src/service/history-impl.js Outdated Show resolved Hide resolved
src/shadow-embed.js Outdated Show resolved Hide resolved
test/functional/test-layout.js Outdated Show resolved Hide resolved
test/functional/test-resource.js Outdated Show resolved Hide resolved
test/functional/test-resource.js Outdated Show resolved Hide resolved
src/shadow-embed.js Outdated Show resolved Hide resolved
Copy link
Contributor

@kristoferbaxter kristoferbaxter left a comment

Choose a reason for hiding this comment

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

One item I believe you should address here, left a few comments.

Copy link
Contributor Author

@rsimha rsimha left a comment

Choose a reason for hiding this comment

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

All comments addressed. PTAL.

babel.config.js Outdated Show resolved Hide resolved
babel.config.js Outdated Show resolved Hide resolved
build-system/tasks/compile-bind-expr.js Outdated Show resolved Hide resolved
build-system/tasks/karma.conf.js Outdated Show resolved Hide resolved
build-system/tasks/update-packages.js Outdated Show resolved Hide resolved
src/error.js Show resolved Hide resolved
src/service/history-impl.js Outdated Show resolved Hide resolved
test/functional/test-layout.js Outdated Show resolved Hide resolved
test/functional/test-resource.js Outdated Show resolved Hide resolved
test/functional/test-resource.js Outdated Show resolved Hide resolved
@rsimha
Copy link
Contributor Author

rsimha commented Oct 10, 2018

@kristoferbaxter I hesitate to make extensive changes to compile-expr.js in this PR (since I'm merely moving code around here to ease the babel upgrade). Okay if we deal with these in a follow up PR? (I'll be sure to have you review it.)

Copy link
Contributor

@kristoferbaxter kristoferbaxter left a comment

Choose a reason for hiding this comment

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

Left a few comments on the compile-expr file.

@kristoferbaxter
Copy link
Contributor

Happy to have a followup PR for compile-expr. Nice work!

build-system/tasks/compile-expr.js Outdated Show resolved Hide resolved
build-system/tasks/compile-expr.js Outdated Show resolved Hide resolved
src/shadow-embed.js Show resolved Hide resolved
Copy link
Contributor Author

@rsimha rsimha left a comment

Choose a reason for hiding this comment

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

@jridgewell Comments addressed. PTAL.

build-system/tasks/compile-expr.js Outdated Show resolved Hide resolved
build-system/tasks/compile-expr.js Outdated Show resolved Hide resolved
@rsimha rsimha merged commit cc4928e into ampproject:master Oct 10, 2018
@rsimha rsimha deleted the 2018-10-04-Babel branch October 10, 2018 19:51
@rsimha
Copy link
Contributor Author

rsimha commented Oct 10, 2018

Merged!

image
https://www.youtube.com/watch?v=40abpedBKK8

noranazmy pushed a commit to noranazmy/amphtml that referenced this pull request Oct 18, 2018
Enriqe pushed a commit to Enriqe/amphtml that referenced this pull request Nov 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants