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 ESM plugins support #1773

Closed
wants to merge 4 commits into from
Closed

Add ESM plugins support #1773

wants to merge 4 commits into from

Conversation

ai
Copy link
Member

@ai ai commented Aug 31, 2022

Closes #1771

@nex3
Copy link
Contributor

nex3 commented Aug 31, 2022

If a plugin is defined like this:

export default (opts) => {
  return {
    postcssPlugin: 'postcss-name',
    Once(root) { /* ... */ }.
  };
};

export const postcss = true;

This doesn't quite work, because setting i = i.default ignores the fact that postcss = true was set on the original value of i. I think this is the logic you want in normalize():

if (i.postcss === true) {
  if (i.__esModule && i.default) {
    i = i.default()
  } else {
    i = i()
  }
} else if (i.postcss) {
  i = i.postcss
}

@ai
Copy link
Member Author

ai commented Sep 1, 2022

The origin idea of ESM plugins is:

const plugin = () => {
  
}
plugin.postcss = true

export default plugin

postcss = true is a mark on top of plugin.

@nex3
Copy link
Contributor

nex3 commented Sep 2, 2022

I think given that the recommended CJS plugin declaration is

module.exports = (opts = {}) => {
  // Plugin creator to check options or prepare caches
  return {
    postcssPlugin: 'PLUGIN NAME'
    // Plugin listeners
  }
}
module.exports.postcss = true

people will expect the ES6 version to be

export default (opts = {}) => {
  // Plugin creator to check options or prepare caches
  return {
    postcssPlugin: 'PLUGIN NAME'
    // Plugin listeners
  }
}
export const postcss = true

@ai
Copy link
Member Author

ai commented Sep 3, 2022

I replaced examples: 0b57283

And add explicit ESM example: e3a6748

@GrapevineLin
Copy link

hi when will it be merged

@ai
Copy link
Member Author

ai commented Nov 1, 2022

@GrapevineLin when somebody will check that it works

@nex3
Copy link
Contributor

nex3 commented Nov 2, 2022

Sorry for letting this fall of my radar, @ai! Here are my test results:

CJS

No options

This case works great!

const postcss = require('postcss');
const plugin = require(...);
postcss([plugin]).process(...);

Options

Passing options to the plugin is pretty annoying. The object loaded by require() isn't a function, so in that case you have to write either

const postcss = require('postcss');
const plugin = require(...);
postcss([plugin.default(...)]).process(...);

or

const postcss = require('postcss');
const plugin = require(...).default;
postcss([plugin.default(...)]).process(...);

both of which expose the implementation of the plugin as an ESM module to the user.

Native ESM

No arguments

Works great!

import postcss from 'postcss';
import plugin from ...;
postcss([plugin]).process(...);

Arguments

Same problem as for CJS: you can't pass arguments directly to plugin() because it's not a function, it's an object with a default field that is itself a function. It is surprising and dismaying to me that Node's native ESM support doesn't recognize the __esModule convention. I found nodejs/node#40891 which covers this in more detail.

TypeScript

No Arguments

I get a type error from this code:

import postcss from 'postcss';
import plugin from ...;
postcss([plugin]).process(...);
test.ts:4:1 - error TS2769: No overload matches this call.
  Overload 1 of 2, '(plugins?: AcceptedPlugin[]): Processor', gave the following error.
    Type '{ (opts?: Options): Plugin; postcss: boolean; }' is not assignable to type 'AcceptedPlugin'.
      Type '{ (opts?: Options): Plugin; postcss: boolean; }' is not assignable to type 'PluginCreator<any>'.
        Types of property 'postcss' are incompatible.
          Type 'boolean' is not assignable to type 'true'.
  Overload 2 of 2, '(...plugins: AcceptedPlugin[]): Processor', gave the following error.
    Argument of type '{ (opts?: Options): Plugin; postcss: boolean; }[]' is not assignable to parameter of type 'AcceptedPlugin'.
      Type '{ (opts?: Options): Plugin; postcss: boolean; }[]' is missing the following properties from type 'Processor': version, plugins, use, process

4 postcss([plugin]).process('a {b: src("c")}').then(result => {
  ~~~~~~~~~~~~~~~~~

  test.ts:4:10
    4 postcss([plugin]).process('a {b: src("c")}').then(result => {
               ~~~~~~
    Did you mean to call this expression?

This could be pretty easily solved though by adding an explicit TypeScript example that suggests writing

import type {Plugin, PluginCreator} from 'postcss';

export interface Options {
  // ...
}

const plugin: PluginCreator<Options> = opts => {
  return {
    postcssPlugin: 'postcss-name',
    Once (root) {
      // Plugin code
    }
  }
}
plugin.postcss = true
export default plugin;

Arguments

This one actually works (once I fix the type annotations)!

import postcss from 'postcss';
import plugin from ...;
postcss([plugin(...)]).process(...);

postcss-cli

This doesn't work at all. It'll probably need to be updated separately to match this new schema.

Conclusion

There's an irreducible problem here where the ESM polyfill simply cannot play nicely with passing arguments to default
exports. This will eventually go away in a beautiful future world when we can start distributing native ESM packages, but that's a long way away. In the meantime, I think there are three potential routes for mitigation:

  1. Push the pain onto users by requiring them to maybe write .default or maybe not, depending on whether they're using CJS or emulated/native ESM. I don't like this option.

  2. Push the pain onto package authors by asking to provide two separate CJS and ESM exports. I believe Node.js supports this, but I need to investigate further to figure out how complex/painful the optimal setup would be, particularly for plugins written in typescript.

  3. Support a new calling convention in PostCSS that doesn't rely on default exports. For example, you could define:

    export type PluginCreator<PluginOptions> = (opts?: PluginOptions): Plugin | Processor;
    
    export type PluginModule<PluginOptions> =
    | {postcssPlugin: PluginCreator<PluginOptions>}
    | PluginCreator<PluginOptions> & {postcss: true}

    ...but this does mean there are two somewhat-different ways to declare plugin modules, and it means all users have to write a more verbose import (although at least it would be consistent across different module systems).

I'll investigate option 2 more and post the results.

@nex3
Copy link
Contributor

nex3 commented Nov 3, 2022

I've poked around some more with conditional exports. Unfortunately, there doesn't seem to be a way to export a given file only for ESM-polyfill users.

So unless PostCSS goes with Option 3 above, I think from a package author's perspective the best option is to accept that the old CommonJS syntax is the only thing that's universally usable and add a shim:

module.exports = require('./build/index').default;

along with "main": "./cjs.js" in package.json. This will essentially undo the EJS polyfill and make the package act like it exports a normal CommonJS module with a module.exports =. You could go a step further and use conditional exports to also export a native ESM module, but there's really not a strong reason to do so since ESM can consume CJS just fine.

@ai My recommendation to you is:

  • Keep the code as-is, since it works about as well as it can given the constraints.
  • Add explicit TypeScript documentation like I described in my earlier comment.
  • Add a note to the ESM/TS section saying "If your plugin takes arguments, you'll also need to create a cjs.js shim and add "main": "./cjs.js" to your package.json."
  • Update postcss-cli to detect __esModule like processor.js does.

@nex3
Copy link
Contributor

nex3 commented Nov 3, 2022

Update: It turns out TypeScript users can avoid a lot of this pain by using export = instead of export default, like so:

import type {PluginCreator} from 'postcss';

namespace plugin {
  export interface Options {
    // ...
  }
}

const plugin: PluginCreator<plugin.Options> = opts => {
  return {
    postcssPlugin: 'postcss-name',
    Once (root) {
      // Plugin code
    }
  }
}
plugin.postcss = true;
export = plugin;

This generates a CommonJS module just like the one you'd write in raw JS, without the need for the wrapper shims I described above. Unfortunately, it's not even vaguely ESM—that export = construct is TypeScript-only. That said, I think this is probably what you should suggest for TypeScript users rather than export default since, in the end, its compiled output matches what you'd write in plain JS.

@ai
Copy link
Member Author

ai commented Nov 10, 2022

Update postcss-cli to detect __esModule like processor.js does.

Unfortunately, postcss-cli is not an only tool with custom config loader. For instance, webpack and some other builders do it too.

Seems like a critical blocker for me.

@GrimeyPickle
Copy link

Check

Copy link

@GrimeyPickle GrimeyPickle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

feature/esm-plugins

@MrHBS
Copy link

MrHBS commented May 15, 2023

How is this going?

@ai
Copy link
Member Author

ai commented Sep 4, 2023

I am closing this PR since we need a different approach

@ai ai closed this Sep 4, 2023
@silverwind
Copy link

Maybe try using dynamic import() for loading plugins? That should seamlessly work for both ESM and CJS.

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.

Document how to declare a PostCSS plugin in an ES6/TypeScript module
6 participants