♻️ Remove build.conf.js #27161

rcebulko · 2020-03-09T20:57:21Z

amp-owners-bot · 2020-03-09T20:57:27Z

Hey @erwinmombay, these files were changed:

build-system/compile/single-pass.js

OWNERS

babel.config.js

jridgewell · 2020-03-09T22:04:58Z

babel.config.js

-    },
-  ],
+  ['@babel/plugin-transform-react-jsx', jsxOpts],
+  localPlugin('transform-inline-configure-component'),


build.conf is only used in dist and check-types. This plugin and below should not be used for test.

Good catch. Some context on testing below:

This is how Karma is set up to run babelify transforms on test code (using browserify):

amphtml/build-system/tasks/karma.conf.js

Lines 75 to 83 in 3488672

browserify: {

watch: true,

debug: true,

fast: true,

basedir: __dirname + '/../../',

transform: [['babelify', BABELIFY_CONFIG]],

// Prevent "cannot find module" errors on Travis. See #14166.

bundleDelay: isTravisBuild() ? 5000 : 1200,

},

It should be possible to enable debugging for browserify and run gulp unit --files <some file> to see the exact config being picked up by babelify.

Would you mind clarifying what about this is borked, given that it is unchanged from when it lived in build.conf.js? This defaultPlugins method is not used for testing AFAICT. karma.conf.js gets BABELIFY_PLUGINS from helpers.js, which looks like this:

const BABELIFY_PLUGINS = { plugins: [ conf.getReplacePlugin(argv.esm || false), conf.getJsonConfigurationPlugin(), ], };

Unless I'm missing something, this PR should be a no-op. As I mention elsewhere I'm happy to clean up/refactor this and anything else (in separate PRs), but I can't tell how this change would have downstream impacts.

Also @rsimha I'm not sure what you were trying to link to there. AFAICT there's no flag that does anything like increase verbosity or display what config is picked up by babelify; here, debug is already set and all that does is add a source map. I could just log the BABELIFY_CONFIG, but It can tell how that's constructed anyway. Was there something specific you had in mind or was your intent to more generally point me at documentation?

If Karma directly uses BABELIFY_PLUGINS and all tests pass with this PR, I think that's good enough.

I was trying to link to a way to print the list of files being transformed. I guess that link was the wrong one. There are two more Karma tricks that might help:

Change ERROR to DEBUG to see what Karma is doing

amphtml/build-system/tasks/karma.conf.js

Line 139 in 286109a

logLevel: 'ERROR',

Change process.stdout.write('.') here to console.log(<file being transformed>)

amphtml/build-system/tasks/runtime-test/runtime-test-base.js

Lines 198 to 204 in 286109a

bundle.on('transform', function(tr) {

if (tr instanceof babelify) {

tr.once('babelify', function() {

process.stdout.write('.');

});

}

});

@jridgewell does this SGTY?

jridgewell · 2020-03-09T22:07:14Z

babel.config.js

+    argv._.includes('dist') || argv._.includes('check-types');
+  const {esm} = argv;
+  const noModuleTarget = {
+    'browsers': isTravisBuild()


We only want this if in single-pass.

Are you saying that's how it works now and something has changed, or that's not how it works now but it should be fixed? [original code]

jridgewell · 2020-03-09T22:07:54Z

babel.config.js

-  // Closure Compiler builds do not use any of the default settings below until its
-  // an esm build. (Both Multipass and Singlepass)
+  // Closure Compiler builds do not use any of the default settings below until
+  // its an esm build. (Both Multipass and Singlepass)
  if (isClosureCompiler && !esm) {


We want to use dist plugins here.

rsimha

Thanks for taking on this work. Some preliminary comments below.

rsimha · 2020-03-10T02:32:04Z

babel.config.js

-    ? ['Last 2 versions', 'safari >= 9']
-    : ['Last 2 versions'],
+
+const jsxOpts = {


I think this refactor is a good opportunity to document each of these constants with a short comment. @jridgewell, @erwinmombay, or I can help with the ones that aren't obvious.

In this case, I felt it made sense to just embed these options directly in the plugin list below, which makes it fairly clear that they are options to the React transformer. SGTY?

The idea of separating things like jsxOpts into constants was to make the main plugin code more readable. Documenting this in a separate PR SGTM.

rsimha · 2020-03-10T02:38:52Z

babel.config.js

-    },
-  ],
+  ['@babel/plugin-transform-react-jsx', jsxOpts],
+  localPlugin('transform-inline-configure-component'),


Good catch. Some context on testing below:

This is how Karma is set up to run babelify transforms on test code (using browserify):

amphtml/build-system/tasks/karma.conf.js

Lines 75 to 83 in 3488672

browserify: {

watch: true,

debug: true,

fast: true,

basedir: __dirname + '/../../',

transform: [['babelify', BABELIFY_CONFIG]],

// Prevent "cannot find module" errors on Travis. See #14166.

bundleDelay: isTravisBuild() ? 5000 : 1200,

},

It should be possible to enable debugging for browserify and run gulp unit --files <some file> to see the exact config being picked up by babelify.

babel.config.js

rsimha · 2020-03-10T02:48:42Z

build-system/tasks/helpers.js

@@ -19,7 +19,7 @@ const babelify = require('babelify');
 const browserify = require('browserify');
 const buffer = require('vinyl-buffer');
 const colors = require('ansi-colors');
-const conf = require('../compile/build.conf');
+const conf = require('../../babel.config');


@jridgewell Based on this documentation, I think we should eventually eliminate the require-ing of babel.config.js and refactor code in files like this. WDYT?

I agree with this; it was odd to see. That being said, I think that's why build.conf.js may have existed separately in the first place: to define those shared methods somewhere for other files to use. It wasn't clear how much boy-scouting was desired here, or if the goal was to just combine them, so I went conservative the first time around. I'd be happy to refactor/clean this up a bit; will defer to your judgement on how much of that should happen here vs. in a follow-up refactor PR.

rsimha

No more blocking comments from me. Will let @jridgewell review and approve the rest.

rsimha · 2020-03-11T19:58:51Z

babel.config.js

-    ? ['Last 2 versions', 'safari >= 9']
-    : ['Last 2 versions'],
+
+const jsxOpts = {


The idea of separating things like jsxOpts into constants was to make the main plugin code more readable. Documenting this in a separate PR SGTM.

rsimha · 2020-03-11T20:42:01Z

babel.config.js

-    },
-  ],
+  ['@babel/plugin-transform-react-jsx', jsxOpts],
+  localPlugin('transform-inline-configure-component'),


If Karma directly uses BABELIFY_PLUGINS and all tests pass with this PR, I think that's good enough.

I was trying to link to a way to print the list of files being transformed. I guess that link was the wrong one. There are two more Karma tricks that might help:

Change ERROR to DEBUG to see what Karma is doing

amphtml/build-system/tasks/karma.conf.js

Line 139 in 286109a

logLevel: 'ERROR',

Change process.stdout.write('.') here to console.log(<file being transformed>)

amphtml/build-system/tasks/runtime-test/runtime-test-base.js

Lines 198 to 204 in 286109a

bundle.on('transform', function(tr) {

if (tr instanceof babelify) {

tr.once('babelify', function() {

process.stdout.write('.');

});

}

});

@jridgewell does this SGTY?

jridgewell

I think the issue is a misunderstanding with my issue request. Right now, we're configuring babel in multiple places, which leads to a lot of confusion on how to enable plugins for everything. I want to remove every single { plugins: [...] } passed to babel.

To do that, we need to move all configuration into babel.config.js. Don't export any extra methods, anything, from this module. babel.transform() will load the file itself, the config file returns the appropriate configuration, and that's it.

We currently have 4 modes (I think): test, build, dist, dist-single-pass. What should be done is to separate these modes into individual, internal, functions. We can then reduce the default export function to a simple:

module.exports = function(api) {
    api.cache(true);
    if (singlePass) return configureSinglePass();
    if (dist) return configureDist();
    if (build) return configureBuild();
    if (test) return configureTest();
    throw new Error('unknown build configuration');
}

rcebulko · 2020-03-11T21:30:13Z

@jridgewell Gotcha, thanks for clarifying. I have a much better picture now of what the goal state looks like.

rsimha · 2020-03-30T22:15:45Z

I've messed with this code a fair bit recently, so I'll pick up from where Ryan left off.

Edit: I had to abandon the approach of using this PR as a starting point. Updated refactor has been merged, so this PR can be closed.

rcebulko added 4 commits March 9, 2020 16:46

Merge build.conf.js into babel.config.js

9aa3311

Update modules requiring build.conf

d06e8e3

Delete build.conf.js

a3ece10

Update owners files referencing build.conf.js

085aed8

googlebot added the cla: yes label Mar 9, 2020

rcebulko commented Mar 9, 2020

View reviewed changes

OWNERS Show resolved Hide resolved

babel.config.js Show resolved Hide resolved

babel.config.js Show resolved Hide resolved

rcebulko requested review from jridgewell and rsimha March 9, 2020 21:00

Make prettier/eslint happy

149b1fd

rcebulko commented Mar 9, 2020

View reviewed changes

babel.config.js Show resolved Hide resolved

jridgewell requested changes Mar 9, 2020

View reviewed changes

rsimha reviewed Mar 10, 2020

View reviewed changes

rsimha reviewed Mar 11, 2020

View reviewed changes

jridgewell requested changes Mar 11, 2020

View reviewed changes

rsimha mentioned this pull request Mar 24, 2020

Babel Following Closure Compilation #26779

Merged

jridgewell mentioned this pull request Mar 26, 2020

🏗🚀 Speed up pre-closure babel transforms #27426

Merged

1 task

rsimha mentioned this pull request Apr 5, 2020

🏗♻️ Consolidate all babel configs into babel.config.js #27576

Merged

rsimha closed this in #27576 Apr 9, 2020

rcebulko deleted the conf branch April 10, 2020 13:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

♻️ Remove build.conf.js #27161

♻️ Remove build.conf.js #27161

rcebulko commented Mar 9, 2020

amp-owners-bot bot commented Mar 9, 2020 •

edited

Loading

jridgewell Mar 9, 2020

rsimha Mar 10, 2020

rcebulko Mar 10, 2020

rsimha Mar 11, 2020

jridgewell Mar 9, 2020

rcebulko Mar 10, 2020

jridgewell Mar 9, 2020

rsimha left a comment

rsimha Mar 10, 2020

rcebulko Mar 10, 2020

rsimha Mar 11, 2020

rsimha Mar 10, 2020

rsimha Mar 10, 2020

rcebulko Mar 10, 2020

rsimha left a comment •

edited

Loading

rsimha Mar 11, 2020

rsimha Mar 11, 2020

jridgewell left a comment

rcebulko commented Mar 11, 2020

rsimha commented Mar 30, 2020 •

edited

Loading

	browserify: {
	watch: true,
	debug: true,
	fast: true,
	basedir: __dirname + '/../../',
	transform: [['babelify', BABELIFY_CONFIG]],
	// Prevent "cannot find module" errors on Travis. See #14166.
	bundleDelay: isTravisBuild() ? 5000 : 1200,
	},

	bundle.on('transform', function(tr) {
	if (tr instanceof babelify) {
	tr.once('babelify', function() {
	process.stdout.write('.');
	});
	}
	});

♻️ Remove build.conf.js #27161

♻️ Remove build.conf.js #27161

Conversation

rcebulko commented Mar 9, 2020

amp-owners-bot bot commented Mar 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rsimha left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rsimha left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jridgewell left a comment

Choose a reason for hiding this comment

rcebulko commented Mar 11, 2020

rsimha commented Mar 30, 2020 • edited Loading

amp-owners-bot bot commented Mar 9, 2020 •

edited

Loading

rsimha left a comment •

edited

Loading

rsimha commented Mar 30, 2020 •

edited

Loading