Build: Ensure that production builds are valid ES5

[ockham]

Fixes #13693.

Changes proposed in this Pull Request:

Add ES5 validation to production builds.

Inspired by Automattic/wp-calypso#36596.

Can't currently add it to the extension build, since there are non-ES5 constructs in them.

Jetpack product discussion

Code quality.

Does this pull request change what data or activity we track or use?

Nah.

Testing instructions:

Run yarn build-production, and verify that eslint doesn't report any errors at the end.

Proposed changelog entry for your changes:

Add ES5 validation to production builds.

[jetpackbot]

Scheduled Jetpack release: November 10, 2020.
Scheduled code freeze: November 3, 2020

E2E results is available here (for debugging purposes): https://jetpack-e2e-dashboard.herokuapp.com/pr-17127

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Generated by 🚫 dangerJS against b2f8b37

[david-szabo97]

Might try --no-inline-config, this flag disables inline eslint comments. I had to do the same for Editing Toolkit since it retains some comments that caused "rules missing" error.

[david-szabo97]

How about running it through terser before validating? That should remove the comments.
I was experimenting with this at Editing Toolkit

npx terser ./editing-toolkit-plugin/*/dist/*.js | npx eslint --parser-options=ecmaVersion:5 --no-eslintrc --no-ignore --stdin

[ockham]

How about running it through terser before validating? That should remove the comments.
I was experimenting with this at Editing Toolkit

npx terser ./editing-toolkit-plugin/*/dist/*.js | npx eslint --parser-options=ecmaVersion:5 --no-eslintrc --no-ignore --stdin

Yeah, so running through terser is what needs to be done, but I don't want to call it directly in package.json. Minification needs to be part of a production build anyway, and if it currently isn't, there's something wrong. terser is invoked as part of calypso-build -- if it's not run here, then there's either a bug, or we're simply not building in production mode.

(I realize that that means that we might have to make sure we only run validation after a production build; but running it unconditionally from package.json might also pose a problem, if we'd run it on an already-minified production build.)

[david-szabo97]

No, it's working right actually 🤔 But validation is at the wrong script.
It should be in the build-production-* scripts.

"build-production": "yarn distclean && yarn install --production=false && yarn build-production-client && yarn build-production-php && NODE_ENV=production yarn build-extensions && NODE_ENV=production yarn build-search && NODE_ENV=production yarn build-packages"

Notice how we don't have a build-production-extensions script but it's inlined: (same goes for build-search and build-packages

NODE_ENV=production yarn build-extensions

Probably we should add a build-production-extensions and add validation in that script. (and we can do the same for search and packages)

"build-production-extensions": "NODE_ENV=production yarn build-extensions  && yarn validate-es5 -- ./_inc/blocks/"

[ockham]

Ohh you're right! Looks like I didn't read that package.json properly 😬

[ockham]

Added validation to the other production builds. The funny part is, those pass validation now, whereas the production build for extensions doesn't 😅

$ npx eslint --parser-options=ecmaVersion:5 --no-eslintrc --no-ignore ./_inc/blocks/

jetpack/_inc/blocks/editor-beta.js
  18:76800  error  Parsing error: The keyword 'const' is reserved

jetpack/_inc/blocks/editor-experimental.js
  18:76800  error  Parsing error: The keyword 'const' is reserved

jetpack/_inc/blocks/editor-no-post-editor.js
  18:75375  error  Parsing error: The keyword 'const' is reserved

jetpack/_inc/blocks/editor.js
  18:76800  error  Parsing error: The keyword 'const' is reserved

[david-szabo97]

Do you remember the PR that added more modules to the transpile list? Well, that's not happening here.

Take a look at these lines: ( and delete them :) )

jetpack/webpack.config.extensions.js

Lines 115 to 123 in 42f04ea

	// The `module` override fixes https://github.com/Automattic/jetpack/issues/12511.
	// @TODO Remove once there's a fix in `@automattic/calypso-build`
	module: {
	...extensionsWebpackConfig.module,
	rules: [
	{ ...transpileConfig, exclude: /node_modules\/(?!punycode)/ },
	..._.without( extensionsWebpackConfig.module.rules, transpileConfig ),
	],
	},

and delete these as well since we aren't using this variable anymore

jetpack/webpack.config.extensions.js

Lines 93 to 95 in 42f04ea

	const transpileConfig = extensionsWebpackConfig.module.rules.find( rule =>
	rule.use.some( loader => loader.options.presets )
	);

That is already fixed in calypso-build so we need to remove those lines to make sure we use the calypso-build's config

[ockham]

Do you remember the PR that added more modules to the transpile list? Well, that's not happening here.

Take a look at these lines: ( and delete them :) )

jetpack/webpack.config.extensions.js

Lines 115 to 123 in 42f04ea

	// The `module` override fixes https://github.com/Automattic/jetpack/issues/12511.
	// @TODO Remove once there's a fix in `@automattic/calypso-build`
	module: {
	...extensionsWebpackConfig.module,
	rules: [
	{ ...transpileConfig, exclude: /node_modules\/(?!punycode)/ },
	..._.without( extensionsWebpackConfig.module.rules, transpileConfig ),
	],
	},

and delete these as well since we aren't using this variable anymore

jetpack/webpack.config.extensions.js

Lines 93 to 95 in 42f04ea

	const transpileConfig = extensionsWebpackConfig.module.rules.find( rule =>
	rule.use.some( loader => loader.options.presets )
	);

That is already fixed in calypso-build so we need to remove those lines to make sure we use the calypso-build's config

Great spot, thanks @david-szabo97! Fix coming right up 😄

[jeherve]

Is it worth waiting until #16845 gets merged so we can add validation to all builds at once?

[ockham]

Is it worth waiting until #16845 gets merged so we can add validation to all builds at once?

Yeah, makes sense 👍

[matticbot]

Caution: This PR has changes that must be merged to WordPress.com
Hello ockham! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer and confirm D51391-code works as expected before merging this PR. Once this PR is merged, please commit the changes to WP.com. Thank you!
This revision will be updated with each commit to this PR

[ockham]

@jeherve Since #16845 apparently requires some more work, can we proceed with this one (and maybe add ES5 validation for the extensions build to #16845)?

[jeherve]

can we proceed with this one

Yeah, sounds like a plan!