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 .cjs file ending for config file locations #596

Merged
merged 6 commits into from Feb 6, 2021

Conversation

nnmrts
Copy link
Contributor

@nnmrts nnmrts commented Feb 2, 2021

Maybe I'm missing something, but I can't get .np-config.js files to work in a folder with type: module in the package.json. This should fix that issue and also isn't inferring with anything else here AFAIK.

I also thought about adding the possibility for .mjs config files, for users without type: module but which want to write their config file in es module syntax. But that can also be in a seperate pull request and I wanted to keep it simple for now.

@sindresorhus
Copy link
Owner

sindresorhus commented Feb 3, 2021

but I can't get .np-config.js files to work in a folder with type: module in the package.json.

Did you look into fixing that? I would prefer to properly fix this instead of adding support for .cjs and .mjs. To keep it simple.

@nnmrts
Copy link
Contributor Author

nnmrts commented Feb 4, 2021

but I can't get .np-config.js files to work in a folder with type: module in the package.json.

Did you look into fixing that? I would prefer to properly fix this instead of adding support for .cjs and .mjs. To keep it simple.

If I don't misunderstand something here, this is a node.js "issue", not an np issue. It's not even an issue though,. With "type": "module" in the package.json, Node assumes every .js file in the same folder to be an ES module, and every .cjs file to be a CommonJS module. And ES modules can import CommonJS modules, but CommonJS modules can't require ES modules. They can import() them. But in this case, np shouldn't, because if I read https://github.com/sindresorhus/np#config correctly, the config should be either in JSON or CommonJS format. So it never actually is an ES module format in the first place, which is why it should use .cjs in ES modules anyway, which also means np should support it (unless np shouldn't support ES modules in general).

https://github.com/sindresorhus/np/blob/main/source/config.js requires the config through cosmiconfig though, which won't change to an import anytime soon, as they've just added .cjs support already too. See this issue: cosmiconfig/cosmiconfig#224

So yeah, theoretically we could "fix" all this by using something else than cosmiconfig and moving to an ES module format for this whole project. But that would just move the problem to another place in the project in an unnecessary complicated way. Just adding .cjs support seemed like an easier and non-breaking fix to me. The default for searchPlaces at cosmiconfig already includes .cjs config files as well, so it just seemed logical to me to use that here as well.

I know all this doesn't seem "proper", un-simple and a little hacky, but in my (and Node.js') mind, using .cjs for CommonJS files in an ESM project is just a more explicit way to indicate the difference to whatever needs to know that difference, in this case Node.js itself.

@sindresorhus
Copy link
Owner

which won't change to an import anytime soon, as they've just added .cjs support already too. See this issue: cosmiconfig/cosmiconfig#224

This is exactly why I was against using cosmiconfig in the first place. I didn't want to end up in a situation like this...

@sindresorhus
Copy link
Owner

#599

@sindresorhus sindresorhus merged commit b49b20f into sindresorhus:main Feb 6, 2021
@sindresorhus
Copy link
Owner

Merging, since you're are right, this is the only feasible workaround for now.

@nnmrts
Copy link
Contributor Author

nnmrts commented Feb 6, 2021

Thanks! Yeah, I feel you. I currently have to use cjs for ALL my config files, babel to eslint to idk what else, because it always breaks somehow if I want to use pure ESM for all of it. Which is a shame obviously, and I absolutely support np going ESM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants