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

Feature Request: support mjs config #230

Closed
xiaoxiangmoe opened this issue Feb 10, 2022 · 28 comments
Closed

Feature Request: support mjs config #230

xiaoxiangmoe opened this issue Feb 10, 2022 · 28 comments

Comments

@xiaoxiangmoe
Copy link

We need support .postcssrc.mjs or postcss.config.mjs.
Also we need support .postcssrc.cjs or postcss.config.cjs.

@ai
Copy link
Member

ai commented Feb 10, 2022

Sure. Send PR.

@xiaoxiangmoe
Copy link
Author

xiaoxiangmoe commented Feb 10, 2022

I found loading esm module is async, it seems it will break sync load config.

https://github.com/postcss/postcss-load-config#sync

Maybe we can only use mjs in async mode. Is this work as intended?

@ai
Copy link
Member

ai commented Feb 10, 2022

We can add .cjs support

@xiaoxiangmoe
Copy link
Author

xiaoxiangmoe commented Feb 10, 2022

What should we do about mjs support?😂

@ai
Copy link
Member

ai commented Feb 10, 2022

What should we do about mjs support?

Sorry, have no idea

@michael42
Copy link
Contributor

michael42 commented Mar 29, 2022

Getting the basic support working shouldn't be hard. With lilconfig@^2.0.5, the following patch mostly works:

diff --git a/src/index.js b/src/index.js
index db5c40e..56a4538 100644
--- a/src/index.js
+++ b/src/index.js
@@ -81,14 +81,19 @@ const addTypeScriptLoader = (options = {}, loader) => {
       `.${moduleName}rc.ts`,
       `.${moduleName}rc.js`,
       `.${moduleName}rc.cjs`,
+      `.${moduleName}rc.mjs`,
       `${moduleName}.config.ts`,
       `${moduleName}.config.js`,
-      `${moduleName}.config.cjs`
+      `${moduleName}.config.cjs`,
+      `${moduleName}.config.mjs`
     ],
     loaders: {
       ...options.loaders,
       '.yaml': (filepath, content) => yaml.parse(content),
       '.yml': (filepath, content) => yaml.parse(content),
+      '.js': (filepath) => import(filepath).then(module => module.default),
+      '.cjs': (filepath) => import(filepath).then(module => module.default),
+      '.mjs': (filepath) => import(filepath).then(module => module.default),
       '.ts': loader
     }
   }

But the details are tricky:

  • I also switched .js to support ECMAScript modues (CommonJS is also supported by import(...)) for projects using "type": "module" in the package.json
  • import(...) requires running jest with NODE_OPTIONS=--experimental-vm-modules
  • import(...) requires at least Node 12, but postcss-load-config still supports Node 10
  • import(...) does not support synchronously loading config files at all

@ai
Copy link
Member

ai commented Mar 29, 2022

Hm. We could think about major release. When is Node 12 end of support?

@michael42
Copy link
Contributor

Node 12 seems to have 2022-04-30 as end of life and Node 10 went EOL on 2021-04-30.

@ai
Copy link
Member

ai commented Mar 29, 2022

Is it possible to check import support in ".js" block and use require for old Node.js versions?

@michael42
Copy link
Contributor

Probably, but it's a new syntax, not just a function, so it requires some sort of eval-based check and would complicate the tests even more. But, on the other hand, it could then be used in the synchronous API, too (albeit only the async API would actually support .mjs and ESM in .js).

@xiaoxiangmoe
Copy link
Author

Is it possible to check import support in ".js" block and use require for old Node.js versions?

Maybe we can check node version

@ai
Copy link
Member

ai commented Mar 29, 2022

Can we make PR to play with this approach?

michael42 added a commit to michael42/postcss-load-config that referenced this issue Mar 29, 2022
@michael42
Copy link
Contributor

I created #234, implementing require with a fallback to import(...). I noticed that (in contrast to my earlier comment import can be easily checked with try-catch block because Node 10 already parses import(...), it just always throws an error (when executing). Only Node 8 does not even parse it. But since that won't work when trying to load a config file in sync mode, I switched it around to prefer require and only use import(...) when necessary.

And just a warning: Node often crashes with a Segmentation fault when running the jest tests in that PR. Not sure why, though, maybe jest exposes a Node/V8 Bug?

Stack trace of thread 47614:
 #0  0x000055aa9c2cbaf0 n/a (node + 0x626af0)
 #1  0x000055aa9c75b7a7 _ZN2v88internal7Isolate38RunHostImportModuleDynamicallyCallbackENS0_6HandleINS0_6ScriptEEENS2_INS0_6ObjectEEENS0_11M>
 #2  0x000055aa9cae66cf _ZN2v88internal25Runtime_DynamicImportCallEiPmPNS0_7IsolateE (node + 0xe416cf)

@ai
Copy link
Member

ai commented Mar 29, 2022

Very strange. On what Node.js versions?

@michael42
Copy link
Contributor

Very strange. On what Node.js versions?

Node 13, 14, 15, 16 and 17. Strangely, it works on Node 12 (node:12-slim docker image) for me.

@ai
Copy link
Member

ai commented Mar 29, 2022

Does it work without Jest? For instance, you can replace postcss-load-config in some project.

@michael42
Copy link
Contributor

I think the (seg fault) issue is nodejs/node#35889, apparently is has something to with how jest hooks into the ESM loading process. I could not get it to crash without jest.

@ai
Copy link
Member

ai commented Mar 30, 2022

I can ask my junior developers to move postcss-load-config from Jest to uvu, but it could take a few weeks.

@ai
Copy link
Member

ai commented Mar 31, 2022

@michael42 postcss-load-config was moved from Jest to uvu by @usmanyunusov

Can you pull changes to your pull request to see the result?

michael42 added a commit to michael42/postcss-load-config that referenced this issue Apr 3, 2022
@michael42
Copy link
Contributor

I just rebased, it seems the crashes are gone, now.

I think the biggest issue now is the synchronous loading mode. By preferring require over import, synchronously loading of CJS files is still supported, but loading ESM configs that way is really messy. Right now it results in a Promise getting passed to processResult and no values from the config are actually being used. I think throwing an error in that case would be clearer.

Further still, why is postcss-load-config even providing synchronous config loading? Loading a config file is inherently async, because it requires disk I/O. Is maintaining that code path worthwhile and are there any downstream consumers of that API?

@ai
Copy link
Member

ai commented Apr 4, 2022

Yes, we can move to async-only API in next major release

michael42 added a commit to michael42/postcss-load-config that referenced this issue Apr 6, 2022
@michael42
Copy link
Contributor

Great, I just pushed another version that removes the sync API. That made it possible to switch to the import-first strategy (that can be simplified to import-only with Node 12) and enabled some further refactorings for the tests. I think it's quite readable now.

All in all, I consider the PR almost complete now. Unfortunately, the coverage dropped below the limit, because of the Node 10 fallback. Not sure if it's worth to add another test for that legacy edge-case, I think excluding that lines from coverage or lowering the limit a bit is more appropriate.

@ai
Copy link
Member

ai commented Apr 6, 2022

It is OK to decrease a limit.

@Rolanddoda
Copy link

Any update on this?

@ai
Copy link
Member

ai commented May 23, 2022

Thanks for the ping. I released in it 4.0.

We will need to wait for PostCSS runners for the update.

@ZerdoX-x
Copy link

ZerdoX-x commented Jul 10, 2022

@ai @michael42 Hi! Thank you for this.

What about postcss.config.ts?

import autoprefixer from "autoprefixer";

const config = {
  plugins: [
    autoprefixer,
  ],
};

export default config;

Config above can't be loaded by postcss-load-config.

Also I want to mention that typescript 4.5 has .mts and .cts file extensions which would be nice to handle (if we handle .mjs and .cjs, why not .mts and .cts)

@ai
Copy link
Member

ai commented Jul 10, 2022

@ZerdoX-x it already should be supported https://github.com/postcss/postcss-load-config/blob/main/src/index.js#L91

@ai ai closed this as completed Jul 10, 2022
@ai
Copy link
Member

ai commented Jul 10, 2022

Also I want to mention that typescript 4.5 has .mts and .cts file extensions which would be nice to handle (if we handle .mjs and .cjs, why not .mts and .cts)

Yes, adding .mts/.cts is a good idea.

Please, send PR.

samcx added a commit to vercel/next.js that referenced this issue Mar 11, 2024
<!-- Thanks for opening a PR! Your contribution is much appreciated.
To make sure your PR is handled as smoothly as possible we request that
you follow the checklist sections below.
Choose the right checklist for the change(s) that you're making:

## For Contributors

### Improving Documentation

- Run `pnpm prettier-fix` to fix formatting issues before opening the
PR.
- Read the Docs Contribution Guide to ensure your contribution follows
the docs guidelines:
https://nextjs.org/docs/community/contribution-guide

### Adding or Updating Examples

- The "examples guidelines" are followed from our contributing doc
https://github.com/vercel/next.js/blob/canary/contributing/examples/adding-examples.md
- Make sure the linting passes by running `pnpm build && pnpm lint`. See
https://github.com/vercel/next.js/blob/canary/contributing/repository/linting.md

### Fixing a bug

- Related issues linked using `fixes #number`
- Tests added. See:
https://github.com/vercel/next.js/blob/canary/contributing/core/testing.md#writing-tests-for-nextjs
- Errors have a helpful link attached, see
https://github.com/vercel/next.js/blob/canary/contributing.md

### Adding a feature

- Implements an existing feature request or RFC. Make sure the feature
request has been accepted for implementation before opening a PR. (A
discussion must be opened, see
https://github.com/vercel/next.js/discussions/new?category=ideas)
- Related issues/discussions are linked using `fixes #number`
- e2e tests added
(https://github.com/vercel/next.js/blob/canary/contributing/core/testing.md#writing-tests-for-nextjs)
- Documentation added
- Telemetry added. In case of a feature if it's used or not.
- Errors have a helpful link attached, see
https://github.com/vercel/next.js/blob/canary/contributing.md


## For Maintainers

- Minimal description (aim for explaining to someone not on the team to
understand the PR)
- When linking to a Slack thread, you might want to share details of the
conclusion
- Link both the Linear (Fixes NEXT-xxx) and the GitHub issues
- Add review comments if necessary to explain to the reviewer the logic
behind a change

### What?

### Why?

### How?

Closes NEXT-
Fixes #

-->


### What?

Prevent confusing error messages when changing to `"type": "module"` in
`package.json`

```
./node_modules/next/dist/build/webpack/loaders/css-loader/src/index.js??ruleSet[1].rules[2].oneOf[8].use[1]!./node_modules/next/dist/build/webpack/loaders/postcss-loader/src/index.js??ruleSet[1].rules[2].oneOf[8].use[2]!./src/styles/index.css
Error [ERR_REQUIRE_ESM]: require() of ES Module /path/to/my/repo/components/postcss.config.js from /path/to/my/repo/components/node_modules/next/dist/lib/find-config.js not supported.
Instead change the require of postcss.config.js in /path/to/my/repo/components/node_modules/next/dist/lib/find-config.js to a dynamic import() which is available in all CommonJS modules.
```

### Why?

Even though PostCSS itself [supports ESM and TypeScript configuration
files](postcss/postcss-load-config#230),
Next.js itself does not (because of `next/lib/find-config`):

- #34448

### How?

By switching to `.cjs`, the config will stay recognized as CommonJS even
after switching to `"type": "module"` in `package.json`

cc @balazsorban44

---------

Co-authored-by: Sam Ko <sam@vercel.com>
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

No branches or pull requests

5 participants