Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add configuration to specify how extensions should be loaded #2345

Closed
novemberborn opened this issue Jan 5, 2020 · 11 comments · Fixed by #2540
Closed

Add configuration to specify how extensions should be loaded #2345

novemberborn opened this issue Jan 5, 2020 · 11 comments · Fixed by #2540
Labels

Comments

@novemberborn
Copy link
Member

We need a way for AVA to know whether to load a particular test file (given its extension) as commonjs or module, similar to the "type" field in package.json.

It must not be possible to change how cjs and mjs extensions are loaded. 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).

Once configured as module, we then need to import() these test files. This is blocked by #2344.

@novemberborn novemberborn added enhancement new functionality help wanted blocked waiting on something else to be resolved first labels Jan 5, 2020
@novemberborn novemberborn mentioned this issue Jan 5, 2020
12 tasks
@novemberborn novemberborn removed the blocked waiting on something else to be resolved first label Feb 9, 2020
@novemberborn
Copy link
Member Author

This is now available to be worked on.

@macarie
Copy link
Contributor

macarie commented Jul 12, 2020

How would you handle this?

Would it break something if AVA just looked at the "type" field in package.json and if type === 'module' && !ref.endsWith('.cjs') used import()?

Today I played around with the Experimental Loaders Hooks and somehow I managed to get everything working (or at least the basics: index.test.ts, with a little tweak to AVA, and import { foo } from './bar.ts').

The only thing that appears to be blocking is this issue because index.test.ts is not loaded as a module and using "typescript": true threw as I'm not using @ava/typescript.

@novemberborn
Copy link
Member Author

How would you handle this?

Would it break something if AVA just looked at the "type" field in package.json and if type === 'module' && !ref.endsWith('.cjs') used import()?

The way I understand it, the "type" field controls whether you need to require() or import() just .js files. It does not control any other extension. This issue proposes that we do support some sort of configuration so that AVA knows to import() rather than require().

.mjs files must always be imported, and .cjs files required.

Today I played around with the Experimental Loaders Hooks and somehow I managed to get everything working

Nice! I imagine you could set those up in a module you can then load using AVA's require option?

@macarie
Copy link
Contributor

macarie commented Jul 12, 2020

The way I understand it, the "type" field controls whether you need to require() or import() just .js files. It does not control any other extension. This issue proposes that we do support some sort of configuration so that AVA knows to import() rather than require().

.mjs files must always be imported, and .cjs files required.

For what I could test, yes, the "type" field defines what happens with .js files. If you try to use require() in a .js file and the package is defined as a module you'd get an error saying that require is not defined, but it would work in the same package if the file was a .cjs one.

The problem is that when you go with one you're locked in with that: in my .ts files I use imports and the transpiler doesn't convert these to requires, hence the error I talked about above.

Having the possibility to define how to load tests with different extensions would certainly be nice, but I'm not really sure how useful that would be. Does it even make sense to have "type": "module" in package.json then test the module using require() (or having something like babel that transforms imports to requires)?

Personally, I'd prefer something that Just Works™ out of the box, so following the "type" field and automagically loading everything I define in ava.extensions with import() if the package is a module (and the file is not .cjs). This is what Node does, so I'd follow that path as it'll become the de facto standard.

If I'd really want to have access to require() in a test maybe a comment at the beginning of the file would be more explicit, something like:

// @ava: load-as-commonjs
const test = require('ava')

Nice! I imagine you could set those up in a module you can then load using AVA's require option?

Yes and no, it is enabled through --experimental-loader {file/module} and it has to be used as "nodeArguments", not really nice to see, maybe having a "loader" field that is mapped to that flag would be a bit better.

Another problem I had is that my loader produces inline sourcemaps, enabling --enable-source-maps works with Node, the stack trace is correct, but AVA doesn't seem to follow it and points the the transpiled lines, but I guess that's a problem for another issue 😋

@novemberborn
Copy link
Member Author

Personally, I'd prefer something that Just Works™ out of the box, so following the "type" field and automagically loading everything I define in ava.extensions with import() if the package is a module (and the file is not .cjs). This is what Node does, so I'd follow that path as it'll become the de facto standard.

But Node.js only does that for .js extensions. If anything, in ESM mode it supports fewer custom things.

I'd like AVA core to not do anything beyond what Node.js does by default. So it should recognize the "type", yes, and beyond that be configurable to import rather than require for custom extensions.

Nice! I imagine you could set those up in a module you can then load using AVA's require option?

Yes and no, it is enabled through --experimental-loader {file/module} and it has to be used as "nodeArguments", not really nice to see, maybe having a "loader" field that is mapped to that flag would be a bit better.

Right — AVA won't enable experimental options. But that's what the nodeArguments configuration is for. As and when these options become stable we may consider friendlier ways of accessing them.

Another problem I had is that my loader produces inline sourcemaps, enabling --enable-source-maps works with Node, the stack trace is correct, but AVA doesn't seem to follow it and points the the transpiled lines, but I guess that's a problem for another issue 😋

Yes. There may be one or two open already around removing our source map magic.

@macarie
Copy link
Contributor

macarie commented Jul 12, 2020

Personally, I'd prefer something that Just Works™ out of the box, so following the "type" field and automagically loading everything I define in ava.extensions with import() if the package is a module (and the file is not .cjs). This is what Node does, so I'd follow that path as it'll become the de facto standard.

But Node.js only does that for .js extensions. If anything, in ESM mode it supports fewer custom things.

I'd like AVA core to not do anything beyond what Node.js does by default. So it should recognize the "type", yes, and beyond that be configurable to import rather than require for custom extensions.

I understand what you're saying, but it does it that way because it's the only type of file it knows how to run beyond .cjs and .mjs.

If you run node --experimental-loader ./ts-loader.js index.ts and in package.json "type" is defined as "module", then index.ts is run as a module, with no extra steps. If "type": "module" is not defined, then running node -r ts-node/register index.ts would run it as CommonJS, thus locking you to only use require() (loaders only work with modules).

That's why I'm saying that AVA should recognize the "type" and use it as its default even for custom file extensions - that's what Node itself does by default - with the possibility to disable that behaviour somehow.

For Node everything still runs as a .js file, the only difference is that it loads the contents of index.ts as a string, passes it around to a bunch of user-defined functions that produce JS code, and then it runs whatever it got from these as if it were a normal index.js file. Basically it's like eval(await transpileWhateverToJS(await readFile('index.ts'))).

I also understand that loading everything as a module when "type": "module" would break existing TS support through ts-node/register because registers don't work with ESM imports (do they? I've tried and it didn't work for me, but maybe I did something wrong).

How should we proceed? The more future-proof way would be to import() everything when "type" is "module" except .cjs test files, but that would break existing things that use the require configuration, the choice of doing it manually through some option wouldn't break anything, but going forward, when loaders won't be experimental anymore, it would be a bit tedious. That's your choice, I'd like to help whatever you choose 🙂

For now we could continue reading the "type" field for .js files and still load everything else with require(), so nothing breaks, then, when loaders will be stable, use import() when "type" is "module" for everything but .cjs files and release this behaviour with a major version. But, for now, how should we define the extensions to be loaded with import()? Maybe something like

{
  "ava": {
    "extensions": {
      ".ts": "module",
      ".re": "commonjs"
    }
  }
}

wouldn't be that bad, we could keep the array and have its children as objects, like [ { ".ts": "module" } ] or even more talkative with [ { "extension": ".ts", "loadAs": "module" } ], do you have any suggestion?

Sorry for these extra-long posts, I kinda like writing 🤣

@novemberborn
Copy link
Member Author

If you run node --experimental-loader ./ts-loader.js index.ts and in package.json "type" is defined as "module", then index.ts is run as a module, with no extra steps.

For my understanding, would index.ts be treated as a module even without the experimental loader? Or is this enabled by the hooks installed through that loader?

I think loaders are too experimental to influence AVA's defaults. If they stabilize and the default behavior is more clear (and breaking) we could enable that as an experimental AVA feature until we ship the next major release.

Your sample configuration looks about right. I'd prefer the object form.

@macarie
Copy link
Contributor

macarie commented Jul 13, 2020

If you run node --experimental-loader ./ts-loader.js index.ts and in package.json "type" is defined as "module", then index.ts is run as a module, with no extra steps.

For my understanding, would index.ts be treated as a module even without the experimental loader? Or is this enabled by the hooks installed through that loader?

I did some more tests, I've removed "type": "module" and run node --experimental-loader ./ts-loader.js -r ts-node/register index.ts, index.ts is loaded as a module with access to import and export, if I remove the loader it'll run as CommonJS, a bit weird, but it seems that if you define a loader the file will load as a module. node -r ts-node/register index.ts doesn't work with "type": "module".

To answer your question: if you don't have "type": "module" and run node index.ts (index.ts being just JS code) it runs as CommonJS, if you have "type": "module" node index.ts won't run, TypeError [ERR_UNKNOWN_FILE_EXTENSION]: Unknown file extension ".ts". So, yes, it would try to load it as a module.

Your sample configuration looks about right. I'd prefer the object form.

Nice! I'll give it a go then. Do you have any advice before I start? 🙂

macarie added a commit to macarie/ava that referenced this issue Jul 14, 2020
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).
@novemberborn
Copy link
Member Author

it seems that if you define a loader the file will load as a module

Is this the case if the loader doesn't specify support for the .ts extension?

All of this is still experimental and subject to change, but I guess it'd be good to understand what's going on here, at this point.

@macarie
Copy link
Contributor

macarie commented Jul 16, 2020

Is this the case if the loader doesn't specify support for the .ts extension?

So, I did more testing, when node [--flags] is called and there is an experimental loader, the getFormat hook is called first, then, based on its output, the files are either loaded as CommonJS or ESM, so the loader decides how it should load files ignoring the "type" field (it's only used as a fallback it seems). On the one hand it kinda makes sense, on the other it doesn't...

Anyway, with #2540 if you use a loader and request ts-node/register and set "ts": "commonjs" it works with require(), so it should be good.

There's a problem tho, if the loader says that it can manage .js extensions as modules subprocess.js will be loaded as a module, and that's not good:

file://[...]/node_modules/ava/lib/worker/subprocess.js:2
const {pathToFileURL} = require("url");

That seems to be caused by fork.js:37, I don't know if there's a fix for that. Maybe loaders should filter out AVA files or AVA itself might have a "loader manager" of some kind when they won't be experimental anymore.

@novemberborn
Copy link
Member Author

So, I did more testing

Thanks for the continued research! I think as long as you can configure AVA to import or require a particular extension then we can use these (experimental) loaders to the fullest.

I don't know if there's a fix for that. Maybe loaders should filter out AVA files or AVA itself might have a "loader manager" of some kind when they won't be experimental anymore.

Some best practices will develop around this. If you're going to co-opt .js extensions you better know what you're doing 😄

macarie added a commit to macarie/ava that referenced this issue Aug 3, 2020
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).
novemberborn added a commit that referenced this issue Aug 23, 2020
Fixes #2345.

Co-authored-by: Mark Wubben <mark@novemberborn.net>
adiSuper94 pushed a commit to adiSuper94/ava that referenced this issue Sep 19, 2020
Fixes avajs#2345.

Co-authored-by: Mark Wubben <mark@novemberborn.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants