Make preset-env skip CommonJS compilation by default, so that Webpack itself can handle it.

[loganfsmyth]

What is the new behavior?

With babel/babel#8485, Babel 7.x now supports a caller option which can pass in metadata about the execution environment.

This PR enables the supportsStaticESM flag, which means that preset-env will automatically default to outputting ES modules instead of automatically converting files to CommonJS.

EDIT

Some users are coming across this behavior and finding that it breaks in specific cases. Please keep in mind that this only affects the default behavior for preset-env, and you can still use the modules argument to configure the behavior yourself.

• modules: "commonjs" will force preset-env to compile ESM to CommonJS
• modules: false will force preset-env to skip ESM compilation.

[hzoo]

Do we need to check the version?

And this should close #650, #617 right? also cc @TheLarkInn

[loganfsmyth]

We already expect Webpack >= 2 in

babel-loader/package.json

Line 20 in 6a70942

"webpack": ">=2"

and I believe Webpack 2.x was the first version to support ES module syntax. If that's not the case, we can definitely check the Webpack version, assuming that's exposed somewhere for us.

[Andarist]

and I believe Webpack 2.x was the first version to support ES module syntax. If that's not the case, we can definitely check the Webpack version, assuming that's exposed somewhere for us.

This is the case, webpack@2 introduced tree-shaking in betas (or before) and we know that it uses ESM to do it.

[Andarist]

maybe add a comment that this can be removed after a while?

[loganfsmyth]

Can do.

[hzoo]

🎉Awesome was just wondering, seems like we don't need that version check then!

Related, but doesn't need to be in this PR but we can also add auto dynamic import syntax?

[loganfsmyth]

Related, but doesn't need to be in this PR but we can also add auto dynamic import syntax?

Yeah, let me look into that too.

[ovidiuch]

Hey, I think I have an issue related to this PR. I upgraded react-cosmos to Babel 7 and a webpack build from the monorepo no longer works.

The webpack build becomes corrupt and generates this error.

Uncaught TypeError: Cannot assign to read only property 'exports' of object '#<Object>'

I didn't find a lot of threads related to it, but for this 2017 comment from @sokra

The code above is ok. You can mix require and export. You can't mix import and module.exports.

This is all vague, but the reason I post it is this: I made this change inside node_modules/babel-loader/lib/transform.js and my build resumed working.

-supportsStaticESM: true,
+supportsStaticESM: false,
-supportsDynamicImport: true,
+supportsDynamicImport: false

And I just upgraded babel-loader, so this PR is likely related.

FYI, my Babel config looks like this:

presets: ['@babel/env', '@babel/react', '@babel/flow'],
plugins: [
  '@babel/plugin-proposal-object-rest-spread',
  '@babel/plugin-proposal-class-properties',
  '@babel/transform-runtime'
],

And it works just fine throughtout the repo where it's picked up by Babel directly. My webpack version is 4.17.1.

Let me know if you have any ideas. Maybe I need to update something else.

[loganfsmyth]

@skidding You'll most likely want to set sourceType: "unambiguous" in your config so that Babel differentiates ES modules and CJS modules the same way that Webpack does. Otherwise, Babel will just assume that every file that it processes is an ES module, which can lead to the issue you're seeing.

[ovidiuch]

@loganfsmyth Thanks for the quick reply!

You'll most likely want to set sourceType: "unambiguous" in your config so that Babel differentiates ES modules and CJS modules the same way that Webpack does.

I tried but it doesn't seem to fix my situation. Only disabling supportsStaticESM and supportsDynamicImport inside babel-loader seems to work for now.

Is there any other information I can provide?

[loganfsmyth]

Is there a branch with clear steps I can use to reproduce? I see https://github.com/react-cosmos/react-cosmos/tree/babel-7 but I'm not sure if that's the one to use of which steps to run to reproduce the error.

[ovidiuch]

Is there a branch with clear steps I can use to reproduce? I see https://github.com/react-cosmos/react-cosmos/tree/babel-7

Yes, that's it. Here are the steps.

yarn # This will likely take a bit 😬
yarn build
yarn start # This will point you to localhost:8989 where you'll be met by the broken build

And here is other useful information.

# Babel config
babel.config.js

# Webpack config
packages/react-cosmos-playground/webpack.config.js

# Cmd to rebuild the webpack build only (with dev env)
yarn webpack --config packages/react-cosmos-playground/webpack.config.js

# Build output path
packages/react-cosmos-playground/dist/index.js

I appreciate your time!

[loganfsmyth]

Ah! Alright, this is an expected error. In https://github.com/react-cosmos/react-cosmos/blob/7ad046f79b20b979cdcbff4155c2aaa8d7817f0b/packages/react-cosmos-playground/src/index.js#L15 you use a mix of import and module.exports, which is not something we'd generally expect.

This happens to work with Babel's module transform because Babel just see's the file as ESM and doesn't even know that module.exports is there.

Webpack on the other hand, since it has to bundle everything, absolutely cares if you use a mix of import and module.exports in the same file, and the previous Babel transform was basically hiding that on accident.

My recommendation would be to use export default there, and if avoiding .default is your goal, use https://webpack.js.org/configuration/output/#output-libraryexport

[ovidiuch]

Alright, this is an expected error. In https://github.com/react-cosmos/react-cosmos/blob/7ad046f79b20b979cdcbff4155c2aaa8d7817f0b/packages/react-cosmos-playground/src/index.js#L15 you use a mix of import and module.exports, which is not something we'd generally expect.

Cool! It was about time that piece of code got updated. Thanks for the libraryExport tip!

But note that the same error is also triggered by a 3rd part dependency. It looks socket.io-client related.

Only together with setting sourceType: "unambiguous" in the Babel config did eveything start working again.

Since the sourceType option doesn't seem to be a popular option (I only found it in the @babel/parser docs), I wonder: Am I still doing something weird, or is everyone bundling socket.io-client expected to use this option from now on? I hope I'm not being too anal about this but I want to make sure I leave with the right lesson from this incident.

[loganfsmyth]

Not that many people run transforms on node_modules, so sourceType isn't usually as necessary. It also doesn't have a huge effect unless you're using preset-env's useBuiltIns or transform-runtime, so often people just don't notice.

Since the sourceType option doesn't seem to be a popular option (I only found it in the @babel/parser docs)

I just landed babel/website#1787 which dramatically expands the docs for Babel's options, so this should help a little: http://babeljs.io/docs/en/next/options#sourcetype

[ovidiuch]

Not that many people run transforms on node_modules

Golden words. I was 100% I wasn't one of those people. But I failed to realize that babel-loader runs the include regular expressions against absolute paths.

Fun fact, this is how I ended up bundling socket.io-client/debug.

Essentially any dependency that had a src dir would've been compiled without my knowing.

This did the trick:

-(react-cosmos.+|react-querystring-router)(\/|\\)src/,
+/(react-cosmos[a-z-]*|react-querystring-router)(\/|\\)src/,

Thanks for bringing this to my attention!

[kaidjohnson]

This thread, specifically #660 (comment), saved a bit of headache looking into why upgrading from 8.0.0-beta.4 to 8.0.0-beta.6 suddenly broke my build. sourceType: 'unambiguous' fixed the issue, and yes, the fringe use-cases discussed here are the use-cases I have, specifically supporting IE8 for a packaged library that is leveraging a few third-party libraries that don't (fully) support IE8, so we end up running the babel transform runtime over everything except core-js.

babel.config.js

module.exports = {
	sourceType: 'unambiguous',
	presets: [
		['@babel/preset-env', {
			useBuiltIns: 'usage',
			debug: false,
			loose: true, // IE8
			exclude: [
				'es6.function.name',
				'es6.regexp.replace',
				'es6.regexp.search',
				'es6.regexp.split',
				'es6.typed.data-view',
				'es6.typed.uint8-array'
			]
		}]
	],
	plugins: [
		'@babel/transform-runtime',
		'transform-es3-member-expression-literals', // IE8
		'transform-es3-property-literals' // IE8
	]
};

and the babel-loader entry in our webpack config:

		rules: [{
			test: /\.js$/,
			exclude: /core-js/,
			loader: 'babel-loader'
		}]

This commit, however, does appear to have created a secondary side-effect that we don't seem to have any control over: unit tests can no longer spy on the import 'default'.

For example:

// a.js
export default function a() {
  ...
}

// b.js
import a from './a.js';
export default function b() {
  a();
}

// b.spec.js
import * as a from './a.js';
import b from './b.js';

describe('myModule', function () {
  beforeEach(function () {
    spyOn(a, 'default');
  });

  it('calls a()', function () {
    b();
    expect(a.default).toHaveBeenCalled();
  });
});

I've been using this approach for a while in unit testing, and have found others that handle it this way as well (https://stackoverflow.com/questions/34113145/how-do-i-use-jasmine-to-spy-on-a-function-that-is-imported-via-an-es6-default-ex, https://stackoverflow.com/questions/35240469/how-to-mock-the-imports-of-an-es6-module/38414108#38414108, https://ozmoroz.com/2017/09/mocking-es6-dependencies-1/).

Upgrading from 8.0.0-beta.4 to 8.0.0-beta.6, this approach no longer works. We get the following error: Error: <spyOn> : default is not declared writable or has no setter. If I manually change supportsStaticESM to false inside node_modules/babel-loader/lib/transform.js, the tests go back to working as before.

My searches led me to babel/babel#8485 and babel/babel#8520 but I can't seem to figure out how to short-circuit the system from the outside (make supportsCallerOptionFlag return false or otherwise reconfigure the caller option with supportsStaticESM: false) in order to get back up and running.

It's possible that we could shift to always exporting an object (even for a single-function module), but this seems like a hacky workaround to get proper access for unit testing.

I hesitate to recommend the ability to configure supportsStaticESM via babel config, as the options are plentiful enough and setting this to false kinda circumvents the goals of the aforementioned tickets. Any thoughts/insights into this issue would be much appreciated.

[loganfsmyth]

@kaidjohnson

What this PR does is change the default for preset-env's modules option from "commonjs" to "auto". If you'd like the old behavior, you can explicitly pass modules: "commonjs" the options, e.g.

['@babel/preset-env', {
  useBuiltIns: 'usage',
  modules: "commonjs",
  // ...

[kaidjohnson]

Thanks @loganfsmyth,

As an experiment, I tried swapping from default export to named export, and the same issue was surfaced: Error: <spyOn> : myProp is not declared writable or has no setter.

I applied modules: 'commonjs' to our test configuration and it does fix the issue. I have some long-term concerns with this approach as I see needing this configuration in all of my projects. I'm curious how supportsStaticESM: true locks the writability of the export name (including default) where using the commonjs fallback doesn't, even though the structure result is the same (_someObj.default).

[loganfsmyth]

I'm curious how supportsStaticESM: true locks the writability of the export name

According to the ECMAScript specification, properties on an ES module namespace object (what you get from import * as a from "";) are read-only. The issue here is that Babel does not implement this read-only restriction and is thus not quite compliant with the spec, but Webpack does make them read-only. Since we now default to letting Webpack handle it, you are getting the spec-compliant behavior, but using spyOn(a, 'default') relies on the non-compliant behavior.

Assuming you're using Jest, you could consider using jest.mock which I think will work even with the spec-compliant behavior?