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

Cannot override config files when used programmatically #501

Closed
novemberborn opened this issue Jul 12, 2018 · 12 comments
Closed

Cannot override config files when used programmatically #501

novemberborn opened this issue Jul 12, 2018 · 12 comments

Comments

@novemberborn
Copy link
Contributor

See avajs/ava#1820 (comment). The user has a project with both AVA (@next) and esm installed. If package.json includes {"esm": {mode: "strict"}} then AVA fails to load the ava.config.js file.

AVA sets the mode to "all" when instantiating esm, but it seems like the package.json configuration still overrides the mode? Reproduced with v3.0.69.

@jdalton
Copy link
Member

jdalton commented Jul 13, 2018

Hi @novemberborn!

Thanks for the heads up. This is kind-of by design, but I think we can work on it. When passing options programmatically to esm the loader is using those options as the defaults but still allows users to provide their own options. This is because folders or sub packages may want to be customized beyond the default options.

In this case though, this is an entry script so it would seem reasonable that you'd want your options provided to the loader to at least apply to the entry script itself (regardless of subsequent module loads from the entry).

({default: fileConf = MISSING_DEFAULT_EXPORT} = esm(module, {
  cjs: false,
  mode: 'all'
})(path.join(projectDir, 'ava.config.js')));

@novemberborn
Copy link
Contributor Author

this is an entry script so it would seem reasonable that you'd want your options provided to the loader to at least apply to the entry script itself (regardless of subsequent module loads from the entry).

Yes that sounds correct, and is what I had assumed the behavior would be. Perhaps an extend: false option (name notwithstanding) could do the trick?

@jdalton
Copy link
Member

jdalton commented Jul 13, 2018

Perhaps an extend: false option (name notwithstanding) could do the trick?

I'll try to make this just-work without adding extra config. My current plan is to use the programmatic options as the defaults for all non-entry scripts (current behavior), but use them as an override for the entry script.

@novemberborn
Copy link
Contributor Author

That would be a breaking change, no?

@jdalton
Copy link
Member

jdalton commented Jul 13, 2018

In the gradient scale of changes I think this would primarily effect folks in AVA's boat. So a test, bundle, or other util that uses esm to load configs. This change is likely something the user would expect to work already (just as you assumed). So from that standpoint it can be seen more as a bug fix, certainly something that I wouldn't mind patch or minor bumping for.

Update:

Thinking on this more. This would fundamentally change how esm handles options. At the moment we handle it in a per package way. A "package", as seen by esm, is any folder with options (specified in a variety of ways). Using the programmatic options via AVA it sets the loader up with defaults. But then the file it loads can be anywhere. In this case it's in the user's project directory to load a config file. The user specifies a different set or rules for loading files which exclude ESM in .js so in this case we're respecting the user's config wishes. I think the better approach is to go the --config-file route at avajs/ava#1857.

Note:

Hi @cantremember 👋,

Node core and Node Module WG member here (me). The .mjs extension is still very experimental so you'll likely continue to run into rough edges with its use. The esm loader locks .mjs use down because we don't know what full support looks like just yet (as it's still being ironed out in Node). There are several possible implementation routes for Node to take, many are not compatible with --experimental-modules today. This means there's a possibility that the code you write today for --experimental-modules will break in the future as things shake out. It's not a bad idea to avoid relying on experimental features for any production code.

@novemberborn
Copy link
Contributor Author

@jdalton that's a pity. I'm not sure we can use esm if we then have to add a disclaimer about what happens when users have configured it in their project.

Config loading also happens early enough that we don't have a way for users to indicate they want to use esm to load an .mjs file, and I don't really want AVA to commit to loading .mjs config files if it doesn't handle .mjs test files.

Is your concern that any imports of the ava.config.js file should be loaded according to the user's settings?

Given that we're using the programmatic API, is there any hook where we can use esm to transpile the config file according to our own settings, so that we can then execute it ourselves through the vm module? The output code could then use the user's settings for any further imports.

@jdalton jdalton removed the wontfix label Jul 13, 2018
@jdalton
Copy link
Member

jdalton commented Jul 13, 2018

I think going down the extend: false (whatever its named) would be the approach to take then since we can't reasonably detect intent here.

@jdalton jdalton reopened this Jul 13, 2018
@novemberborn
Copy link
Contributor Author

Thanks!

How would that impact imports from the file itself?

@jdalton
Copy link
Member

jdalton commented Jul 13, 2018

For that loader instance I would simply disable resolving esm options so it would stick with the defaults provided by the programmatic API. So loading in the file itself would be dictated by whatever the programmatic options were.

Update:

I have this working locally. Bikeshed option names? I currently have configurable as the option but I'm open to others. It should express toggling respecting user provided options. Maybe fixed or immutable or locked or force.

Update:

I went with force 🙃

@cantremember
Copy link

cantremember commented Jul 14, 2018

@novemberborn thank you for bringing this issue over into esm 👍

@jdalton i completely understand -- '.mjs' is experimental. it just happens to be the most practical way to identify that a particular script is ESM-flavored. it makes a lot of sense for an evolving adoption process. i deeply appreciate that a project like esm is out there in the wild to enable folks like me to begin that process without a transpilation step.

i could have (a) turned off strict if i had needed to. but keeping it on serves me much better than (b) being a Purist about how i express my ava config -- package.json gave me a backup plan, and i was able to stay strict. my avajs/ava#1820 (comment) was my observation that their CLIs doesn't allow the user to specify a config filename -- which would have allowed me to also be a Purist (a habit i'm trying to break)

💯

@novemberborn
Copy link
Contributor Author

@jdalton great! This works insofar as that AVA can load ava.config.js even if the project uses mode: 'strict', but the project settings do not apply to imports from the ava.config.js file, at least as far as mode: 'strict' is concerned. I do seem to be able to import CJS modules despite AVA setting cjs: false so maybe this is just a mode issue?

@jdalton
Copy link
Member

jdalton commented Jul 14, 2018

The cjs:false option doesn't disallow CJS it just turns off all CJS interop features. The mode:'strict' allows importing CJS files. They are handled as --experimental-modules handles them, as a single default export (no named exports support).

Update:

Tests bdf8feb;

Update:

v3.0.71 is released 🎉

jdalton added a commit that referenced this issue Jul 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

3 participants