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

Update babel dependencies #10810

Merged
merged 13 commits into from Mar 26, 2019
Merged

Update babel dependencies #10810

merged 13 commits into from Mar 26, 2019

Conversation

renovate[bot]
Copy link
Contributor

@renovate renovate bot commented Dec 4, 2018

This PR contains the following updates:

Package Type Update Change References
babel-loader dependencies major 7.1.5 -> 8.0.4 source

Release Notes

babel/babel-loader

v8.0.4

Compare Source

v8.0.3

Compare Source

Features
Bugs
  • #​685 - Also pass the caller option to loadPartialConfig
Docs
  • #​681 - Update the README links to use the new options docs
  • #​683 - Add .mjs to the examples
Internal

Some dev dependency updates and CI tweaks.

v8.0.2

Compare Source

  • #​541 - A bunch of great README fixes
  • #​574 - Add cacheCompression: false to disable GZip compression of the disk cache
  • #​670 - Handle both 'sourceMap' and 'sourceMaps' options properly
  • #​671 - Fix sourceMaps: 'inline' to work properly with babel-loader
  • #​669 - Fix sourcemaps to work with Webpack's devtoolModuleFilenameTemplate placeholders

v8.0.1

Compare Source

  • #​662 - docs: update README.md
  • #​667 - docs: Remove babelrc from loader-specific options
  • #​668 - Add a warning if you forget to install @babel/core or install babel-core.

v8.0.0

Compare Source

This is the first stable release of babel-loader for Babel 7.x.

  • README updates
  • Dropped peer dependency on betas and RCs, but left the backward-compat code we had for now to give people time to migrate

Renovate configuration

📅 Schedule: At any time (no schedule defined).

🚦 Automerge: Disabled by config. Please merge this manually once you are satisfied.

♻️ Rebasing: Whenever PR becomes conflicted, or if you modify the PR title to begin with "rebase!".

🔕 Ignore: Close this PR and you won't be reminded about this update again.


  • If you want to rebase/retry this PR, check this box

This PR has been generated by Renovate Bot. View repository job log here.

@renovate renovate bot requested a review from a team as a code owner December 4, 2018 17:02
@renovate renovate bot added [Status] Needs Review To request a review from Crew. Label will be renamed soon. [Type] Janitorial labels Dec 4, 2018
@jetpackbot
Copy link

jetpackbot commented Dec 4, 2018

Warnings
⚠️ "Testing instructions" are missing for this PR. Please add some
⚠️ "Proposed changelog entry" is missing for this PR. Please include any meaningful changes

This is an automated check which relies on PULL_REQUEST_TEMPLATE. We encourage you to follow that template as it helps Jetpack maintainers do their job. If you think 'Testing instructions' or 'Proposed changelog entry' are not needed for your PR - please explain why you think so. Thanks for cooperation 🤖

Generated by 🚫 dangerJS against 6aaa2d9

brbrr
brbrr previously requested changes Dec 5, 2018
Copy link
Contributor

@brbrr brbrr left a comment

Choose a reason for hiding this comment

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

Build failed locally. Needs to investigate it further.

Message:
    Module build failed: Error: Cannot find module '@babel/core'
 babel-loader@8 requires Babel 7.x (the package '@babel/core'). If you'd like to use Babel 6.x ('babel-core'), you should install 'babel-loader@7'.
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:580:15)
    at Function.Module._load (internal/modules/cjs/loader.js:506:25)
    at Module.require (internal/modules/cjs/loader.js:636:17)
    at require (internal/modules/cjs/helpers.js:20:18)
    at Object.<anonymous> (/Users/brbrr/Developer/a8c/jetpack/node_modules/babel-loader/lib/index.js:10:11)
    at Module._compile (internal/modules/cjs/loader.js:688:30)
    at Module._extensions..js (internal/modules/cjs/loader.js:699:10)
    at Object.require.extensions.(anonymous function) [as .js] (/Users/brbrr/Developer/a8c/jetpack/node_modules/babel-register/lib/node.js:152:7)
    at Module.load (internal/modules/cjs/loader.js:598:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:537:12)
Details:
    module: Module[1000]
    error: Error: Cannot find module '@babel/core'
 babel-loader@8 requires Babel 7.x (the package '@babel/core'). If you'd like to use Babel 6.x ('babel-core'), you should install 'babel-loader@7'.
    origin: null
    dependencies: null

@renovate
Copy link
Contributor Author

renovate bot commented Dec 7, 2018

PR has been edited

👷 This PR has received other commits, so Renovate will stop updating it to avoid conflicts or other problems. If you wish to abandon your changes and have Renovate start over you may click the "rebase" checkbox in the PR body/description.

@brbrr brbrr changed the title Update dependency babel-loader to v8 Update babel dependencies Dec 7, 2018
@sirreal
Copy link
Member

sirreal commented Dec 10, 2018

This is related to some conversations I had recently while working with @Automattic/team-calypso.

One area of current work is publishing the parts of Calypso that are useful cross-organization or to a wider audience. One idea that came up was to export a core, opinionated, generic, build/babel/jsx/transpilation package.

That wouldn't solve the issues present in this PR, but it could help alleviate some overhead maintaining separate build systems for Calypso and Jetpack's dashboard that have pretty much the same requirements.

@jsnajdr
Copy link
Member

jsnajdr commented Dec 12, 2018

@sirreal I see that the Jetpack Babel config includes Stage 1 proposals. Is it some legacy from Babel 6, or are any of the non-standard JS features used in Jetpack code?

First step to unify the Babel configs is to agree on one version of the language 🙂 Calypso uses standard ES2018 + JSX + class properties. Gutenberg is even more conservative, avoiding class properties syntax and having JSX as the only language extension.

@brbrr
Copy link
Contributor

brbrr commented Dec 12, 2018

It is legacy that I moved from "old" config. Actually, I haven't checked that we still need these non-standard features. that's a good point - will be good to review usage.

@sirreal
Copy link
Member

sirreal commented Dec 12, 2018

First step to unify the Babel configs is to agree on one version of the language

You mean a completely different language :trollface:? How about whatever Calypso defines? That would be another way we can help to standardize across projects. Developers know what to expect and if you have motivation to change, there's a centralized place to propose it.

It aligns with the linter config/plugin moving into Calypso. We can make sure Jetpack code is compatible with whatever config Calypso provides.

@brbrr
Copy link
Contributor

brbrr commented Dec 13, 2018

How about whatever Calypso defines?

I like that. We already copied some of the practices from Calypso into Jetpack (and other projects), which makes total sense for me to have some centralized extendable configuration.

One thing that I not sure about - it who should take it home. I am more than happy to work on this, but I will need to learn a lot :)

@sirreal
Copy link
Member

sirreal commented Dec 13, 2018

One thing that I not sure about - it who should take it home.

I don't know either. Could a published, modularized build config be on a short-term @Automattic/team-calypso roadmap? What about providing guidance and direction for folks that can take this on like @brbrr?

cc: @ehg for devx visibility

@jsnajdr
Copy link
Member

jsnajdr commented Dec 13, 2018

I like the idea to publish a standard Babel config out of the Calypso repo and import it in other projects like Jetpack. These projects won't need to build the config from scratch, but will just override the defaults where needed (and if needed).

The first step now should be to clean up the Jetpack Babel config. Upgrade to Babel 7. Remove the transforms that we don't need, especially the Stage 1 ones. Don't duplicate the config in webpack.config.js and .babelrc.

Only as a step two, we'll be able to see the differences between Calypso and Jetpack clearly (maybe there won't be any?) and create a common package.

Team Calypso can help here a lot. We've been through a Babel upgrade in Calypso already and cooperated with Gutenberg folks a lot. We know the details and gotchas of many Babel packages and transforms and options -- there's quite a learning curve there 🙂

@brbrr
Copy link
Contributor

brbrr commented Dec 14, 2018

Thanks! I'll re-visit Babel config, and will try to update to v7 next week

@jsnajdr
Copy link
Member

jsnajdr commented Dec 18, 2018

@brbrr @oskosk I tried to finish the Babel 7 upgrade and to make the config as simple as possible. The Babel config now does just a few things:

  • use the env preset to transpile latest standard ES2018 to code compatible with supported browsers
  • use the react preset for JSX
  • use the class properties plugin to support the nonstandard class syntax
  • use the transform-runtime plugin to deduplicate Babel helper code

Can you do some testing and let me know if you find anything suspicious? I tested yarn build-client and build finished successfully.

I had to patch several module reexports in the state directory to avoid adding more nonstandard extensions. The following:

// module.js
export default { foo };
// user.js
import { foo } from 'module';

is not correct ES modules code. foo is not a named export of module. It's a property of the default export object. The same thing in CommonJS, not the same thing in ESM.

Another opportunity for simplification: rename gulpfile.babel.js to gulpfile.js and convert import statements in the gulpfile.js to requires that Node.js understand. That will stop transpilation of Gulpfile with Babel and will let us avoid the modules: 'commonjs' line in Babel config. If we transpile to ES modules instead of CommonJS, webpack will be able to do tree shaking and other magical optimizations.

Looking forward to upgrade webpack to v4 as a next step 🙂

@jsnajdr
Copy link
Member

jsnajdr commented Dec 18, 2018

There are ESLint and test failures on this branch. I'm working on them -- trying to fix everything without upgrading ESLint and the whole universe in one PR.

@jsnajdr
Copy link
Member

jsnajdr commented Dec 19, 2018

Hi @brbrr and @jeherve 👋 While trying to resolve the remaining problems, I stumbled into a rabbit hole 😄 Jetpack repo uses a very old ESLint that's not compatible with Babel 7 (whose parser it uses via the babel-eslint package). When trying to upgrade ESLint to the latest 5.10, I get a lot of linter errors about code formatting.

To get out of this rabbit hole and finish the upgrades, it would help me if the codebase got reformatted with Prettier. That solves a lot of the formatting bugs. And then we can add the eslint-config-prettier plugin to the ESLint config, which disables all the formatting-related ESLint rules. We do that in Calypso already: https://github.com/Automattic/wp-calypso/blob/master/.eslintrc.js#L11-L12

Doing these changes makes a clean and error-free ESLint upgrade possible. What do you think?

@jeherve
Copy link
Member

jeherve commented Dec 19, 2018

it would help me if the codebase got reformatted with Prettier.

That would be nice indeed, I am all for prettier things :) (related: #10784), but I have no experience with running Prettier on a whole codebase and how that would look like. How did it work for Calypso? Should we go at it in multiple PRs, one per file, one per section?

@jsnajdr
Copy link
Member

jsnajdr commented Dec 19, 2018

How did it work for Calypso? Should we go at it in multiple PRs, one per file, one per section?

Calypso was the first Automattic project to use Prettier, so we went mostly per section and debugged our fork (that adds WordPress-style paren spacing) on the way.

For Jetpack repo, I think it's best to do it in one PR. I experimented a bit in my working copy and ended up with two questions:

  • do we want to format SCSS styles, too? Maybe it's safer to start with JS first. Prettier is rock-stable for JavaScript, but for SCSS there are still a few bugs.
  • there are many 3rd party files in modules/. Which of these do we want to format and which should be ignored? I assume that the safest option is to format just _inc/client, as that's the only directory yarn lint checks, too.

@jeherve
Copy link
Member

jeherve commented Dec 19, 2018

Yes, I would agree about limiting things to _inc/client for now. We don't want to run this outside of the admin UI interface, as we have many files that still use ES5 outside of that directory, and we don't really want Prettier to "fix" those files as they are not transpiled anywhere.

do we want to format SCSS styles, too?

How are things configured in Calypso, and if you enabled Prettier for SCSS there, did you do it at the same time as enabling it for JS, or later?

@jsnajdr
Copy link
Member

jsnajdr commented Dec 19, 2018

How are things configured in Calypso, and if you enabled Prettier for SCSS there, did you do it at the same time as enabling it for JS, or later?

We enabled it later, and still haven't reformatted everything. When trying to format SCSS files in #11007, I encountered some errors that made me decide to postpone SCSS for later.

I filed #11007 to do the reformat. Some extra nontrivial steps needed to be taken before it worked 100%.

@brbrr brbrr dismissed their stale review December 21, 2018 06:56

not relevant anymore

@dereksmart dereksmart added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels Feb 20, 2019
@simison
Copy link
Member

simison commented Mar 22, 2019

it would help me if the codebase got reformatted with Prettier.

PR: #11507

simison and others added 10 commits March 26, 2019 16:52
Exporting an object as default doesn't make the object properties into named exports.
That's a CommonJS backward compatibility hack and is not valid ES module syntax.
Converting lets us drop the `add-module-exports` Babel plugin.
- remove unneeded plugins from Babel config
- add the ES3-compat transform (member properties, literals, reserved words)
- remove the duplicate webpack config bits
@sirreal
Copy link
Member

sirreal commented Mar 26, 2019

Rebased now that #11677 has landed with the add-module-exports reverted. Let's give this some testing.

@sirreal sirreal added [Status] Needs Review To request a review from Crew. Label will be renamed soon. [Status] Needs Testing We need to add this change to the testing call for this month's release and removed [Status] In Progress labels Mar 26, 2019
Copy link
Member

@sirreal sirreal left a comment

Choose a reason for hiding this comment

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

Works in my testing 👍

Copy link
Contributor

@ockham ockham left a comment

Choose a reason for hiding this comment

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

Yep, testing fine & code's looking great!

Copy link
Contributor

@kraftbj kraftbj left a comment

Choose a reason for hiding this comment

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

Nice! Installed and built without error. I merged it with master with all of the gutenblock source and build work, built fine, admin dashboard works without issue in the console and blocks worked. Let's do it.

@kraftbj kraftbj added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels Mar 26, 2019
@kraftbj kraftbj merged commit c57b375 into master Mar 26, 2019
@ghost ghost removed [Status] Needs Changelog [Status] Ready to Merge Go ahead, you can push that green button! labels Mar 26, 2019
@kraftbj kraftbj deleted the renovate/babel-loader-8.x branch March 26, 2019 16:58
@jeherve jeherve removed the [Status] Needs Testing We need to add this change to the testing call for this month's release label Apr 27, 2021
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.

None yet