un-ts / remark-preset-prettier

Turns off all rules that are unnecessary or might conflict with Prettier.
MIT License
12 stars 2 forks source link

Not working with ESM #61

Closed ybiquitous closed 3 years ago

ybiquitous commented 3 years ago

Hi! Thank you for providing this nice preset!

I found a behavior that this preset is not work with ESM. It occurs with the latest version remark-preset-prettier@0.5.1.

Let me show for the details.

Reproduction

Prepare the following files.

package.json:

{
  "type": "module",
  "dependencies": {
    "remark-cli": "10.0.0",
    "remark-preset-lint-markdown-style-guide": "5.0.0",
    "remark-preset-prettier": "0.5.1"
  },
  "scripts": {
    "remark": "remark ."
  }
}

.remarkrc.js:

export default {
  plugins: [
    'remark-preset-lint-markdown-style-guide',
    'remark-preset-prettier',
  ],
};

test.md:

-   a
-   b

Run:

$ npm run remark

> remark
> remark .

test.md
  1:5  warning  Incorrect list-item indent: remove 2 spaces  list-item-indent  remark-lint
  2:5  warning  Incorrect list-item indent: remove 2 spaces  list-item-indent  remark-lint

⚠ 2 warnings

I expect no warnings, but 2 warnings are detected.

When I inspected node_modules/remark-preset-prettier/lib/index.js, I found an ignored exception (_a) at line 43:

    try {
        plugins.push([cjsRequire('remark-lint-' + plugin), false]);
    }
    catch (_a) { }
    return plugins;

This ignored exception's message is something like:

Error [ERR_REQUIRE_ESM]: require() of ES Module /private/tmp/foo/node_modules/remark-lint-unordered-list-marker-style/index.js from /private/tmp/foo/node_modules/remark-preset-prettier/lib/index.js not supported.
Instead change the require of /private/tmp/foo/node_modules/remark-lint-unordered-list-marker-style/index.js in /private/tmp/foo/node_modules/remark-preset-prettier/lib/index.js to a dynamic import() which is available in all CommonJS modules.

Maybe, this exception is related to this behavior. 🤔

Environment


If the reproduction above or my understandings are incorrect, please feel free to tell me or close this issue. Thank you.

JounQin commented 3 years ago

Thank you, I'll take a look soon.

So remark-lint-list-item-indent is esm only now.

JounQin commented 3 years ago

@wooorm Hi, I need help here. Is that possible to use await import() in a remark preset? Or do I have to list all possible deps in package.json and use import statement instead?

JounQin commented 3 years ago

@ybiquitous Please try the following version for testing:

yarn add https://pkg.csb.dev/JounQin/remark-preset-prettier/commit/45ec5d24/remark-preset-prettier
npm i https://pkg.csb.dev/JounQin/remark-preset-prettier/commit/45ec5d24/remark-preset-prettier

Although I don't think this is the perfect solution because it may install unexpected remark-lint plugins for end users.

wooorm commented 3 years ago

Is that possible to use await import() in a remark preset?

No, the configuration stage is sync. This is a similar issue to: https://github.com/remarkjs/remark-retext/issues/14.

Or do I have to list all possible deps in package.json and use import statement instead?

Yes. But: you could use createRequire from node:module if you only care about Node: https://github.com/remarkjs/remark/blob/b767382a29994200ca2ae3cc4ad9ecf36decf91f/packages/remark-cli/cli.js#L2.

But I think importing the plugins you want to turn off might be best

ybiquitous commented 3 years ago

Please try the following version for testing:

@JounQin Thanks. The version works expectedly! 👍🏼

JounQin commented 3 years ago

@wooorm

Yes. But: you could use createRequire from node:module if you only care about Node: remarkjs/remark@b767382/packages/remark-cli/cli.js#L2.

I'm using createRequire indeed: https://github.com/JounQin/remark-preset-prettier/blob/master/index.ts#L3-L4

But cjsRequire will just fail on native esm plugin as this issue described because remark-lint-list-item-indent is native esm only:

Error [ERR_REQUIRE_ESM]: require() of ES Module /private/tmp/foo/node_modules/remark-lint-unordered-list-marker-style/index.js from /private/tmp/foo/node_modules/remark-preset-prettier/lib/index.js not supported.
Instead change the require of /private/tmp/foo/node_modules/remark-lint-unordered-list-marker-style/index.js in /private/tmp/foo/node_modules/remark-preset-prettier/lib/index.js to a dynamic import() which is available in all CommonJS modules.

But I think importing the plugins you want to turn off might be best

As I say:

it may install unexpected remark-lint plugins for end users.

For end users not using latest remark-lint plugins, that would be broken. What means it can not be compatible any more.

wooorm commented 3 years ago

For end users not using latest remark-lint plugins, that would be broken. What means it can not be compatible any more.

Yes. Updating dependencies and publishing a major version would solve that though?

JounQin commented 3 years ago

Yes. Updating dependencies and publishing a major version would solve that though?

Well, if there is no better way to fix this, I have to do like that. 😂

Although I still hope there will be better solution like https://github.com/remarkjs/remark-retext/issues/14 or supporting await import() in preset.

wooorm commented 3 years ago

Well, top-level await is supported in Node 14 and 16?

JounQin commented 3 years ago

@ybiquitous I've just released remark-preset-prettier@v1.0.0.

ybiquitous commented 3 years ago

@JounQin Thank you so much for the quick fix! I can confirm the new version 1.0.0 works well. 🥰

JounQin commented 3 years ago

@wooorm As https://github.com/stylelint/remark-preset/pull/6/files#diff-4cfcf9277f6a14080f988dbe6158fedb46b1049c56bc815e42a2736e0b215c70R6, I found [pluginStr, false] would still work, and because of false, even if pluginStr is actually non-existed, it will still work!

export default {
  plugins: [['non-existed-dep', false]],
}

Is this valid? If so, I can still remove all dependencies.

wooorm commented 3 years ago

Strings are supported in configuration files of the unified engine (so in .remarkrc.js), but not in presets like remark-preset-prettier. It is valid in a .remarkrc but it’s not valid in a preset

JounQin commented 3 years ago

@wooorm

Strings are supported in configuration files of the unified engine (so in .remarkrc.js), but not in presets like remark-preset-prettier. It is valid in a .remarkrc but it’s not valid in a preset

Do you mean @ybiquitous's changes https://github.com/stylelint/remark-preset/pull/6/files#diff-4cfcf9277f6a14080f988dbe6158fedb46b1049c56bc815e42a2736e0b215c70R6 is incorrect actually?

But I tried like following and it does work:

export default {
  plugins: [
    'preset-lint-consistent',
    'preset-lint-markdown-style-guide',
    'preset-lint-recommended',
    './lib/index.js',
  ],
}

Is there anything I missed?

wooorm commented 3 years ago

No, that change is good. Because it’s in a .remarkrc file.

Your change is not good, as it is not in a .remarkrc file.

JounQin commented 3 years ago

@stylelint/remark-preset is also a sharable remark-preset.

See https://github.com/stylelint/remark-preset/pull/6/files#diff-e727e4bdf3657fd1d798edcd6b099d6e092f8573cba266154583a746bba0f346R1

wooorm commented 3 years ago

Then that change is incorrect: it does not work

JounQin commented 3 years ago

Then that change is incorrect: it does not work

cc @ybiquitous


@wooorm

I have an idea to create a package named remark-config-prettier to be used in remarkrc only.

Maybe @stylelint/remark-preset is only used in that way too, the preset is confusing then.

Will string config contains string plugins in remarkrc work?

{
  "plugins": ["@stylelint/remark-preset"]
}
wooorm commented 3 years ago

Only one rc file is allowed. They can be shared (put in node_modules/).

However, I strongly recommend against that. Presets work on the API. Can be bundled in a browser. Config files and strings don’t This conversation is getting close to: https://github.com/remarkjs/remark-retext/issues/14.

Will string config contains string plugins in remarkrc work?

That works in an rc file. It does not work in a preset

JounQin commented 3 years ago

Only one rc file is allowed. They can be shared (put in node_modules/).

That works in an rc file. It does not work in a preset

I'm still not sure to understand, I mean if @stylelint/remark-preset changes like that PR, will a .remarkrc in another project work?

// .remarkrc
{
  "plugins": ["@stylelint/remark-preset"]
}

However, I strongly recommend against that. Presets work on the API. Can be bundled in a browser. Config files and strings don’t

I understand that, I'm talking another package which maybe only aim to remarkrc config on Node.js. remark-preset-prettier will not change like that.

wooorm commented 3 years ago

No, I don’t think it will work. That PR is I believe broken.

I understand that, I'm talking another package which maybe only aim to remarkrc config on Node.js. remark-preset-prettier will not change like that.

I’m not sure how it could be used. It could not be combined with other presets. It just turns stuff off. So why not use no rc file at all?

JounQin commented 3 years ago

No, I don’t think it will work. That PR is I believe broken.

OK, if it does not work, remark-config-prettier I proposed is meaningless too maybe.

I was thinking the usage:

{
  "plugins": [
    // ... othere presets
    "config-prettier" // It just turns stuff off for prettier.
  ]
}
ybiquitous commented 3 years ago

@JounQin @wooorm Thank you so much for your help! 🥰

ybiquitous commented 3 years ago

@JounQin @wooorm Thanks again! We were able to release @stylelint/remark-preset@3.0.0 successfully. 😄