support @std/esm #1618

jamestalmage · 2017-12-23T21:55:43Z

This fixes support for @std/esm.

jamestalmage · 2017-12-23T22:17:34Z

lib/test-worker.js

-(opts.require || []).forEach(x => require(x));
+(opts.require || []).forEach(x => {
+	if (/.*std[/\\]esm[/\\]index\.js$/.test(x)) {
+		require = require(x)(module); // eslint-disable-line no-global-assign


Basically, we are making sure that we use @std/esm's require function instead of the standard one when we load the test file later in the file.

I am a bit surprised that this is necessary, since I figured @std/esm would install a require extension hook (via pirates or similar). @jdalton, am I mistaken here? Do you not implement a require hook via require.extensions['.js']?

Since @std/esm is meant to be a production dependency, as well as a dev dependency, we can't just always overload the global require.extensions (the Node folks would flip-out, esp. since Lodash v5 will be using @std/esm).

If @std/esm is invoked through a CLI then it will hook into require.extensions, otherwise it produces its own loader instance for folks to use as you've done 👆. Whenever AVA requires @std/esm through the --require option the @std/esm loader is in CLI mode so will hook require.extensions. In this case, the thing preventing its use is the precompile check which dead-ends the load chain and prevents it from getting to @std/esm.

precompile would only be short circuiting imports for tests, but those will have already used Babel to convert the import statements to require statements.

I poked around in the @std/esm codebase, and didn't see where you were actually hooking require.extensions (even in "CLI mode").

precompile would only be short circuiting imports for tests, but those will have already used Babel to convert the import statements to require statements.

That's the only issue affecting @std/esm (the test files).

I poked around in the @std/esm codebase, and didn't see where you were actually hooking require.extensions (even in "CLI mode").

I think @std/esm might be OK with just having ababel: false or precompile: false option. That way it avoids any package specific sniffs and is something that can be applied for other uses/reasons.

OK, so this is actually only a problem when Babel is configured not to transpile ESM modules. Setting babel:false does not actually disable Babel - we still apply a few transforms to the tests for power-assert support, and providing better errors for common misuses of the API.

Since we're always going to load Babel, precompile: false becomes a huge performance hit, as Babel just takes too long to load.

OK, so this is actually only a problem when Babel is configured not to transpile ESM modules.

Correct. The user wanted to just use @std/esm which is where I pointed to #709.

we still apply a few transforms to the tests for power-assert support

power-assert ships with a UMD build (no Babel required). The babel-plugin-throws-helper could always be optional or could be written for acorn+magic-string or other lighter solutions.

power-assert ships with a UMD build (no Babel required). The babel-plugin-throws-helper could always be optional or could be written for acorn+magic-string or other lighter solutions.

Interesting ideas. Power assert already has an acorn based transform, so the whole stack could share that.

@std/esm uses acorn+magic-string 😋

jdalton · 2017-12-24T00:00:43Z

test/fixture/std-esm/.esmrc

+{
+	"cjs": true,
+	"esm": "js"
+}


👆 You can shorthand to just "cjs" in the .esmrc file. In the next release you can use { "esm":"cjs" } as a shorthand for the above too.

Like "cjs"\n is the full contents of the file?

Ya, kinda gross but it works. You may want to wait for the other shorthand form.

novemberborn

@jamestalmage just so I understand, this detects the @std/esm require and replaces the require() method we use for other "require" values?

Is there a remaining issue with how we load the transpiled test files, or how we transpile them to begin with?

novemberborn · 2017-12-24T12:37:37Z

lib/test-worker.js


-(opts.require || []).forEach(x => require(x));
+(opts.require || []).forEach(x => {
+	if (/.*std[/\\]esm[/\\]index\.js$/.test(x)) {


This should start with [/\\]@std I think, to better match just @std/esm.

jamestalmage · 2017-12-24T17:58:26Z

@novemberborn - The only other issue would be providing a way to bypass the main thread precompilation. In that case, the package sniffing I am doing wouldn't be needed.

jamestalmage · 2017-12-24T18:00:06Z

It also replaces "require" for when we load the tests.

billyjanitsch · 2017-12-24T18:08:13Z

(Apologies for the notification spam.) As the original reporter of standard-things/esm#197, I just want to express appreciation for everyone working on this. Always awesome to see OSS projects come together to support interoperability for users. Thanks y'all ❤️

novemberborn

@novemberborn - The only other issue would be providing a way to bypass the main thread precompilation. In that case, the package sniffing I am doing wouldn't be needed.

That's tracked elsewhere, but in any case it's entirely conceivable that users may want to use @std/esm while still using AVA's transpilations.

I think we should still tighten up the pattern match, and per @jdalton's comments could clean up the .esmrc file.

I know you're generally quite busy @jamestalmage (so thanks for chiming in with this PR!). Let me know if you'd like me to land it instead.

jdalton · 2017-12-28T17:02:42Z

I just want to express appreciation for everyone working on this. Always awesome to see OSS projects come together to support interoperability for users. Thanks y'all

I was pretty floored by everyone jumping in to help out. It's such a pleasant surprise to have to rein in adjustments. Double thanks to y'all ❤️

jdalton · 2018-01-06T02:15:23Z

Okay, @std/esm v0.19.0 is released 🎉 (which supports that shorthand mentioned above)!

Requested changes made.

jamestalmage commented Dec 23, 2017

View reviewed changes

jamestalmage mentioned this pull request Dec 23, 2017

AVA requiring @std/esm does not work standard-things/esm#197

Closed

jdalton reviewed Dec 24, 2017

View reviewed changes

motss mentioned this pull request Dec 24, 2017

Add ES Modules recipe #1593

Merged

novemberborn reviewed Dec 24, 2017

View reviewed changes

novemberborn previously requested changes Dec 28, 2017

View reviewed changes

jamestalmage added 4 commits January 8, 2018 14:15

support @std/esm

d3dd1ce

fix path test for windows

08f1985

fix linter error

2485fbb

Stricter regex. Use shorthand .esmrc file in fixture.

ab8296a

jamestalmage force-pushed the esm-support branch from f3cd328 to ab8296a Compare January 8, 2018 19:15

update package-lock.json

09d7400

jamestalmage merged commit 72c53be into master Jan 8, 2018

sindresorhus deleted the esm-support branch January 8, 2018 20:27

renovate bot mentioned this pull request Mar 8, 2018

chore(deps): update dependency ava to v0.25.0 mono-js/mono-push#4

Merged

support @std/esm #1618

support @std/esm #1618

Uh oh!

Conversation

jamestalmage commented Dec 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jdalton Dec 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jdalton Dec 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jdalton Dec 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamestalmage Dec 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

novemberborn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamestalmage commented Dec 24, 2017

Uh oh!

jamestalmage commented Dec 24, 2017

Uh oh!

billyjanitsch commented Dec 24, 2017

Uh oh!

novemberborn left a comment

Choose a reason for hiding this comment

Uh oh!

jdalton commented Dec 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jdalton commented Jan 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jdalton Dec 23, 2017 •

edited

Loading

jdalton Dec 24, 2017 •

edited

Loading

jdalton Dec 24, 2017 •

edited

Loading

jamestalmage Dec 24, 2017 •

edited

Loading

jdalton commented Dec 28, 2017 •

edited

Loading

jdalton commented Jan 6, 2018 •

edited

Loading