postcss / postcss-load-config

Autoload Config for PostCSS
MIT License
638 stars 71 forks source link

Feature Request: support mjs config #230

Closed xiaoxiangmoe closed 2 years ago

xiaoxiangmoe commented 2 years ago

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

ai commented 2 years ago

Sure. Send PR.

xiaoxiangmoe commented 2 years ago

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 commented 2 years ago

We can add .cjs support

xiaoxiangmoe commented 2 years ago

What should we do about mjs support?😂

ai commented 2 years ago

What should we do about mjs support?

Sorry, have no idea

michael42 commented 2 years ago

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:

ai commented 2 years ago

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

michael42 commented 2 years ago

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

ai commented 2 years ago

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

michael42 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

Can we make PR to play with this approach?

michael42 commented 2 years ago

I created https://github.com/postcss/postcss-load-config/pull/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 commented 2 years ago

Very strange. On what Node.js versions?

michael42 commented 2 years ago

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 commented 2 years ago

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

michael42 commented 2 years ago

I think the (seg fault) issue is https://github.com/nodejs/node/issues/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 commented 2 years ago

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

ai commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

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

michael42 commented 2 years ago

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 commented 2 years ago

It is OK to decrease a limit.

Rolanddoda commented 2 years ago

Any update on this?

ai commented 2 years ago

Thanks for the ping. I released in it 4.0.

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

ZerdoX-x commented 2 years ago

@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 commented 2 years ago

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

ai commented 2 years ago

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.