Experimentally configure module formats for test files #2540

macarie · 2020-07-14T17:09:23Z

Should resolve #2345 by adding an object form for the "extensions" configuration.

And I have a strong suspicion (but I have not confirmed this) that it's impossible to override the "type" field (which applies to js extensions).

I did some tests and it doesn't work, if "type" is "module" you can't require() a js file from that package, nor the opposite, if you try Node gets angry:

require() of [...]/index.test.js from [...]/node_modules/ava/lib/worker/subprocess.js is an ES module file as it is a .js file whose nearest parent package.json contains "type": "module" which defines all .js files in that package scope as ES modules.

I've tried to keep the edits as small and self-contained as possibile. I'm not sure if what I did and how I did it is the right approach, but it seems to be working.

This is how AVA behaves now:

If "extensions" is not defined the default module configuration for mjs, cjs, and js is used.
If "extensions" is an array the module types are generated automatically for each extension, keeping the default type for mjs, cjs, and js, only if they're defined, and using "commonjs" for everything else, thus maintaining backwards compatibility.
If "extensions" is an object the module types are read from the configuration, but the type for mjs, cjs, and js is locked and can only be set to their default type, the default type for js is based on the "type" field because it cannot be overridden.

Now, in package.json (or in a config file), it's possible to have:

{
  "ava": {
    "extensions": {
      "ts": "module",
      "js": "commonjs",
      "cjs": "commonjs",
      "mjs": "module"
    }
  }
}

If you try to change js to "module" (notice the missing "type") it'll throw an error:

  ✖ .js files can only be configured as ’commonjs’ when the nearest parent package.json does not contain "type": "module", found ’module’ instead.

For cjs and mjs files:

  ✖ .cjs files can only be configured as ’commonjs’, found ’module’ instead.

Local tests are passing, not with Node v14.5.0 tho. I don't know why they fail (something with the reporters I guess), but they also fail on master, so I'm not sure what's going on.

I didn't write tests yet, and no documentation either, if what I did looks good I'll do those next 👌🏼

lib/module-types.js

novemberborn · 2020-07-15T17:54:27Z

Thanks for the PR! It may take me a few more days before I can have a good look at this, sorry.

novemberborn · 2020-07-19T16:56:59Z

@macarie I wasn't online much this weekend so I haven't been able to look at this yet, sorry.

novemberborn

Looks good so far @macarie!

You didn't mention it, but we still need to use this configuration to load the test file right?

I think we should make this an opt-in experimental feature for now, but we can deal with that when we're ready to ship this.

lib/module-types.js

macarie · 2020-07-26T20:58:32Z

You didn't mention it, but we still need to use this configuration to load the test file right?

I'm not sure I understand what you are asking, what do you mean?

novemberborn · 2020-07-27T06:48:33Z

You didn't mention it, but we still need to use this configuration to load the test file right?

I'm not sure I understand what you are asking, what do you mean?

Never mind, I'd forgotten how these module types are used.

macarie · 2020-07-27T18:30:43Z

I did the changes you proposed, now it only accepts true for 'js' | 'cjs' | 'mjs' and 'commonjs' | 'module' for everything else.

I don't really like the error messages, something else that might work?

An extension's module type can only be one of ("commonjs" | "module"), found "ts": "modul" instead.

.cjs, .mjs, and .js files can only be enabled using ’true’ because their default import type cannot be overridden, found "js": "module" instead.

novemberborn

This is looking pretty good @macarie! I need to play with this regarding the error messages.

Regarding the tests, would you be up for writing integration-style tests? See https://github.com/avajs/ava/tree/master/test — it's all a bit fresh but the idea is to set up a fixture project with (in this case) the configuration you want to test, and then running AVA in that project and checking the results.

lib/module-types.js

Fixes avajs#2345 by adding an object form for the `"extensions"` configuration. This implementation: 1. if `"extensions"` is not defined uses the default module configuration for `mjs`, `cjs`, and `js` 2. generates the module types from the array form of the `extensions` configuration keeping the default module type for `mjs`, `cjs`, and `js` if they're defined and uses `"commonjs"` for everything else, thus maintaining backwords compatibility. 3. generates the module types from the object form of the `extensions` configuration and prevents changing the module type for `mjs`, `cjs`, and `js` extensions (for `js` the `"type"` field cannot be overridden).

…n types for everything else

macarie · 2020-08-03T23:08:17Z

@novemberborn I've tried doing some tests.

Node v10 fails because I've used an mjs file, not sure how to make it work tho, setting nodeArguments: ['--experimental-modules'] in ava-std.config.js doesn't seem to do anything.

The ts test files are "fake" and not loaded as TS, require() doesn't care about their extension and still treats them as JS, do you want to have something more realistic (= loads real TS files with types)?

I wouldn't try setting anything as "module" because for non-standard extensions we need a loader, and that's still not stable.

As always, any feedback is welcome 🙂

novemberborn · 2020-08-04T08:21:52Z

Node v10 fails because I've used an mjs file, not sure how to make it work tho, setting nodeArguments: ['--experimental-modules'] in ava-std.config.js doesn't seem to do anything.

Just on this, have a look at this branch: https://github.com/avajs/ava/blob/shared-worker-plugins/ava.config.js

It's OK to skip these tests in Node.js 10.

…odule"

novemberborn · 2020-08-08T16:31:12Z

@macarie cool!

I'm about to push some tweaks. My apologies for the refactoring, hopefully you agree it's an improvement and not just me being silly 😄

You took a good stab at the tests. I think it's better to use separate test files, and then use that to exclude ESM tests from Node.js 10. Snapshots should be used with caution, it's easy to think you're testing something even though you aren't. I've also renamed some files.

I've made the feature experimental for now, using the configurableFileLoading flag. How does that name sound to you?

Your feedback on my changes aside, I think what's left here now is to update the documentation?

Use a more explicit switch statement to clarify the logic.

* Use separate test files for better grouping * Prefer assertions over snapshots * But do snapshot error messages * Stop AVA from running tests that require ESM on Node.js 10 * Avoid the esm package

macarie · 2020-08-08T20:15:07Z

Nice work @novemberborn!

My apologies for the refactoring, hopefully you agree it's an improvement and not just me being silly 😄

Yeah, everything's more straightforward now and a bit easier to follow, more Go-ish I'd say 😝

I've made the feature experimental for now, using the configurableFileLoading flag. How does that name sound to you?

Not sure about it. I think it gives more the idea that you can configure what files to load and not how to load them 🤔
Maybe configurableFileFormatLoading or something along this line would be better.

I think what's left here now is to update the documentation?

Yes! Do you want to write it or should I give it a shot?

lib/module-types.js

novemberborn · 2020-08-10T08:13:33Z

Yeah, everything's more straightforward now and a bit easier to follow, more Go-ish I'd say 😝

Please don't tell my coworkers 😉

I've made the feature experimental for now, using the configurableFileLoading flag. How does that name sound to you?

Not sure about it. I think it gives more the idea that you can configure what files to load and not how to load them 🤔
Maybe configurableFileFormatLoading or something along this line would be better.

How about configurableModuleFormat?

What if this default behaviour was an option?

Defaults to 'commonjs' — keeping backwards compatibility — but also configurable as 'module'.

I'd like to follow the package.json "type" option for the time being. If you can write your sources in ESM then you should write your tests the same way.

novemberborn · 2020-08-10T08:14:02Z

I think what's left here now is to update the documentation?

Yes! Do you want to write it or should I give it a shot?

It'd be great if you could give it a shot first.

macarie · 2020-08-10T08:24:41Z

How about configurableModuleFormat?

This sounds good to me.

I'd like to follow the package.json "type" option for the time being. If you can write your sources in ESM then you should write your tests the same way.

Sorry, I was talking about line 50, the default for "unrecognized" extensions, not the "js" default type. I'm not used to GitHub's review tool and apparently the line I click on is not the one in focus.

It'd be great if you could give it a shot first.

Sure 🙂

novemberborn · 2020-08-10T08:42:35Z

I think commonjs is a good default for now. ESM is still very new.

macarie · 2020-08-20T20:31:24Z

Sorry for the delay, it's been a busy few weeks.

Anyway... I've updated the documentation, I wasn't sure where to write about the new feature tho, so I chose 06-configuration.md. Is there a better place?

I've also changed configurableFileLoading to the one you have proposed, configurableModuleFormat, and changed the test's folder name, if it doesn't look right I can revert the commit.

novemberborn · 2020-08-22T12:15:32Z

I wasn't sure where to write about the new feature tho, so I chose 06-configuration.md. Is there a better place?

No I think that's great. I'm about to push some changes to make it a little clearer what this is for. Let me know what you think.

I've also changed configurableFileLoading to the one you have proposed, configurableModuleFormat, and changed the test's folder name

👍

novemberborn · 2020-08-22T12:15:57Z

@macarie I believe this is ready now?

docs/06-configuration.md

macarie · 2020-08-22T14:46:43Z

@novemberborn other than the small change I proposed (feel free to ignore it) I think that this is ready, yes 😄

Co-authored-by: Raul <raul@macarie.me>

novemberborn · 2020-08-23T09:44:59Z

🎊 thanks @macarie! I'll try and ship a release later today.

novemberborn · 2020-08-23T19:39:46Z

3.12 is now available.

macarie commented Jul 14, 2020

View reviewed changes

lib/module-types.js Outdated Show resolved Hide resolved

macarie mentioned this pull request Jul 16, 2020

Add configuration to specify how extensions should be loaded #2345

Closed

novemberborn reviewed Jul 26, 2020

View reviewed changes

lib/module-types.js Outdated Show resolved Hide resolved

lib/module-types.js Outdated Show resolved Hide resolved

novemberborn reviewed Aug 3, 2020

View reviewed changes

lib/module-types.js Outdated Show resolved Hide resolved

lib/module-types.js Outdated Show resolved Hide resolved

macarie added 6 commits August 3, 2020 22:45

Accept only true for default extensions and throw when using unknow…

7246c86

…n types for everything else

Remove console.log

a57d33e

Use ! instead of false equality

7e2d482

Disable import/no-extraneous-dependencies in test files

dcda621

Add integration tests

5d953fe

macarie force-pushed the load-extension-as branch from 01d02ae to 5d953fe Compare August 3, 2020 22:28

macarie and others added 4 commits August 4, 2020 19:31

Disable mjs test on Node < v12

d2149c2

Add integration test for custom extension not set as "commonjs" or "m…

d425d8b

…odule"

Merge branch 'master' into load-extension-as

6087591

Tweaks

0a13b86

novemberborn added 5 commits August 8, 2020 18:31

Refactor module-types.js

6c00297

Use a more explicit switch statement to clarify the logic.

Rename test directory

d7e8dbe

Refactor tests

9894f36

* Use separate test files for better grouping * Prefer assertions over snapshots * But do snapshot error messages * Stop AVA from running tests that require ESM on Node.js 10 * Avoid the esm package

Full code coverage

7982264

Make experimental

2e3784a

macarie commented Aug 8, 2020

View reviewed changes

lib/module-types.js Show resolved Hide resolved

macarie added 2 commits August 20, 2020 20:32

Update documentation

b0476ac

Rename configurableFileLoading to configurableModuleFormat

8d24632

Tweak docs

9ffafe3

novemberborn marked this pull request as ready for review August 22, 2020 12:15

macarie commented Aug 22, 2020

View reviewed changes

docs/06-configuration.md Outdated Show resolved Hide resolved

novemberborn and others added 3 commits August 23, 2020 11:33

Update docs/06-configuration.md

675d782

Co-authored-by: Raul <raul@macarie.me>

One last tweak

cdf4f1b

Merge branch 'master' into load-extension-as

ecbf9d3

novemberborn approved these changes Aug 23, 2020

View reviewed changes

novemberborn changed the title ~~Add possibility to configure how extensions should be loaded~~ Experimentally configure module formats for test files Aug 23, 2020

novemberborn merged commit 5c9dbb9 into avajs:master Aug 23, 2020

This was referenced Sep 24, 2020

CommonJS support via esm HydreIO/rgraph#15

Closed

Update our handling of the require option to support ESM dependencies #2347

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Experimentally configure module formats for test files #2540

Experimentally configure module formats for test files #2540

macarie commented Jul 14, 2020

novemberborn commented Jul 15, 2020

novemberborn commented Jul 19, 2020

novemberborn left a comment

macarie commented Jul 26, 2020

novemberborn commented Jul 27, 2020

macarie commented Jul 27, 2020

novemberborn left a comment

macarie commented Aug 3, 2020

novemberborn commented Aug 4, 2020

novemberborn commented Aug 8, 2020

macarie commented Aug 8, 2020

novemberborn commented Aug 10, 2020

novemberborn commented Aug 10, 2020

macarie commented Aug 10, 2020

novemberborn commented Aug 10, 2020

macarie commented Aug 20, 2020

novemberborn commented Aug 22, 2020

novemberborn commented Aug 22, 2020

macarie commented Aug 22, 2020

novemberborn commented Aug 23, 2020

novemberborn commented Aug 23, 2020

Experimentally configure module formats for test files #2540

Experimentally configure module formats for test files #2540

Conversation

macarie commented Jul 14, 2020

novemberborn commented Jul 15, 2020

novemberborn commented Jul 19, 2020

novemberborn left a comment

Choose a reason for hiding this comment

macarie commented Jul 26, 2020

novemberborn commented Jul 27, 2020

macarie commented Jul 27, 2020

novemberborn left a comment

Choose a reason for hiding this comment

macarie commented Aug 3, 2020

novemberborn commented Aug 4, 2020

novemberborn commented Aug 8, 2020

macarie commented Aug 8, 2020

novemberborn commented Aug 10, 2020

novemberborn commented Aug 10, 2020

macarie commented Aug 10, 2020

novemberborn commented Aug 10, 2020

macarie commented Aug 20, 2020

novemberborn commented Aug 22, 2020

novemberborn commented Aug 22, 2020

macarie commented Aug 22, 2020

novemberborn commented Aug 23, 2020

novemberborn commented Aug 23, 2020