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

Error: Cannot parse file `package.json` with node v12.16.3 npm 6.14.14 #55

Closed scscgit closed 3 years ago

scscgit commented 3 years ago

Environment:

package.json, for example after creating a project via npx create-nuxt-app remarktest and installing remark-cli@9 (because version 10 of remark-cli causes a different Must use import to load ES Module error):

{
  "name": "remarktest",
  "version": "1.0.0",
  "private": true,
  "scripts": {
    "dev": "nuxt",
    "build": "nuxt build",
    "start": "nuxt start",
    "lint:md": "remark --ext .md,.mdx --ignore-path .gitignore --frail --quiet .",
    "generate": "nuxt generate"
  },
  "dependencies": {
    "core-js": "^3.15.1",
    "nuxt": "^2.15.7"
  },
  "remarkConfig": {
    "plugins": [
      "preset-prettier"
    ]
  },
  "devDependencies": {
    "remark-cli": "^9.0.0",
    "remark-preset-prettier": "^0.4.1"
  }
}

When running npm run lint:md (which can be simplified to remark README.md), the following error is logged:

README.md
  1:1  error  Error: Cannot parse file `package.json`
Error: Expected preset or plugin, not undefined, at `node_modules\remark-preset-prettier\lib\cjs.js`
    at Error (C:\ProjectsExternal\remarktest\node_modules\fault\index.js:29:12)
    at onparse (C:\ProjectsExternal\remarktest\node_modules\unified-engine\lib\find-up.js:152:13)
    at done (C:\ProjectsExternal\remarktest\node_modules\trough\wrap.js:55:16)

× 1 error
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! remarktest@1.0.0 l: `remark README.md`
npm ERR! Exit status 1
npm ERR!
npm ERR! Failed at the remarktest@1.0.0 l script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.
JounQin commented 3 years ago

Can you please provide a minimal runnable reproduction?

scscgit commented 3 years ago

It behaves the same with the following package.json based on npm init:

{
  "name": "rema",
  "version": "1.0.0",
  "description": "remark repro for Error: Cannot parse file `package.json`",
  "scripts": {
    "test": "remark README.md"
  },
  "remarkConfig": {
    "plugins": [
      "preset-prettier"
    ]
  },
  "dependencies": {
    "remark-cli": "^9.0.0",
    "remark-preset-prettier": "^0.4.1"
  }
}

echo test > README.md npm test

Here is a project with generated package-lock.json: repro.zip

Also my environment is:

Microsoft Windows 10 Enterprise, OS Version: 10.0.19042

However, this seems to work correctly under virtualized Linux like WSL / Docker. The error also occurs on Git Bash under Windows.

JounQin commented 3 years ago

Then you may raise issue at remark-cli instead. I don't think there is anything related to this package.(No package.json accessing)

And AFAIK, a lot of remark-lint plugins are still cjs

https://github.com/remarkjs/remark-lint/blob/main/packages/remark-lint-blockquote-indentation/index.js

cc @wooorm

Do you know any possible related codes?


Besides, @scscgit you'd better create a git repo for reproduction instead.

wooorm commented 3 years ago

@JounQin im guessing that this project can't be imported from node (esm), because a file extension is missing?

wooorm commented 3 years ago

You could try importing it from an esm file. I think it'll give the same error.

scscgit commented 3 years ago

Yeah I posted here only because this is the only plugin that causes the issue, as I'm using

  "remarkConfig": {
    "plugins": [
      "frontmatter",
      "preset-lint-recommended",
      "preset-prettier"
    ]
  },

I think a repo is an overkill for project that has only 1 package.json file (unless it's caused by lock file), but I'd be very interested in any options along the lines of CI test automation with a matrix of multiple OSes (which I think projects like remark should start using more often for example projects, as a way of enabling users instant repro), or sandbox solutions. Sadly I don't have experience with setting up such test environments from scratch, and I don't have time to experiment too much, as this is a non-critical issue for my project (debugging and isolating this root cause was already exhausting). Feel free to suggest any specific debugging tips, thanks.

Also I've been told that this issue disappears on latest node version, but I don't have exact details (or bisection attempts).

Can you provide an example for that esm import?

wooorm commented 3 years ago

This does seem to work:

node --input-type module --eval 'import * as x from "remark-preset-prettier"; console.log(x)'
[Module: null prototype] {
  __esModule: true,
  default: {
    plugins: [
      [Array], [Array], [Array],
      [Array], [Array], [Array],
...

so it could also be nuxt related

scscgit commented 3 years ago

node 12 requires --experimental-modules

node --experimental-modules --input-type module --eval 'import * as x from "remark-preset-prettier"; console.log(x)'

(node:18360) ExperimentalWarning: The ESM module loader is experimental.
file:///C:/ProjectsExternal/rema/[eval1]:1
'import
^^^^^^^
SyntaxError: Invalid or unexpected token
    at Loader.evalInstance (internal/modules/esm/loader.js:156:22)
    at new ModuleJob (internal/modules/esm/module_job.js:32:41)
    at Loader.eval (internal/modules/esm/loader.js:165:17)
    at internal/process/execution.js:46:37
    at processTicksAndRejections (internal/process/task_queues.js:97:5)

The second package.json doesn't contain nuxt and still fails.

JounQin commented 3 years ago

@wooorm As @scscgit said, this issue is only Windows related, so I guess this should be same as https://github.com/antonk52/lilconfig/issues/17


Sorry, seems similar, but not same exactly.

JounQin commented 3 years ago

node 12 requires --experimental-modules

You should upgrade to latest v12 version.

scscgit commented 3 years ago

I also forgot to mention that this suddenly stopped working even on project commits where it had previously worked, i.e. it retroactively impacted old installation of dependencies, also using the same local node/npm. We also noticed a behavior that after installing latest node and then downgrading back, the new project installation (node_modules) kept working on the lower node version. But I don't have a more detailed report about that at this moment.

C:\ProjectsExternal\rema>node --version
v12.22.4

C:\ProjectsExternal\rema>npm test

> rema@1.0.0 test C:\ProjectsExternal\rema
> remark README.md

README.md
  1:1  error  Error: Cannot parse file `package.json`
Error: Expected preset or plugin, not undefined, at `node_modules\remark-preset-prettier\lib\cjs.js`
    at Error (C:\ProjectsExternal\rema\node_modules\fault\index.js:29:12)
    at onparse (C:\ProjectsExternal\rema\node_modules\unified-engine\lib\find-up.js:152:13)
    at done (C:\ProjectsExternal\rema\node_modules\trough\wrap.js:55:16)

× 1 error
npm ERR! Test failed.  See above for more details.

C:\ProjectsExternal\rema>node --input-type module --eval 'import * as x from "remark-preset-prettier"; console.log(x)'
file:///C:/ProjectsExternal/rema/[eval1]:1
'import
^^^^^^^
SyntaxError: Invalid or unexpected token
    at Loader.evalInstance (internal/modules/esm/loader.js:162:22)
    at new ModuleJob (internal/modules/esm/module_job.js:37:41)
    at Loader.eval (internal/modules/esm/loader.js:171:17)
    at internal/process/execution.js:46:37
JounQin commented 3 years ago

@scscgit Can you please upgrade node first? import a cjs package should just be fine.

scscgit commented 3 years ago

I upgraded using https://nodejs.org/dist/latest-v12.x/ as you see in the first command node --version

JounQin commented 3 years ago

Then that seems a different behavior of Node on Windows, I don't know whether that is expected or a bug of Node itself.

wooorm commented 3 years ago

Maybe it’s fixed on 14 or 16? @scscgit Could you try that?

JounQin commented 3 years ago

I just tried Node v16 on Windows + Git bash, it just works fine.

image

scscgit commented 3 years ago

Note that I'm using remark-cli@9 and not 10. (Also I won't be able to use project with node 16, as I require npm 6 and there are various nuxt issues otherwise.)

Node 14 didn't help

C:\ProjectsExternal\rema>node --version
v14.17.4

C:\ProjectsExternal\rema>npm test

> rema@1.0.0 test C:\ProjectsExternal\rema
> remark README.md

README.md
  1:1  error  Error: Cannot parse file `package.json`
Error: Expected preset or plugin, not undefined, at `node_modules\remark-preset-prettier\lib\cjs.js`
    at Error (C:\ProjectsExternal\rema\node_modules\fault\index.js:29:12)
    at onparse (C:\ProjectsExternal\rema\node_modules\unified-engine\lib\find-up.js:152:13)
    at done (C:\ProjectsExternal\rema\node_modules\trough\wrap.js:55:16)

× 1 error
npm ERR! Test failed.  See above for more details.

C:\ProjectsExternal\rema>node --input-type module --eval 'import * as x from "remark-preset-prettier"; console.log(x)'
file:///C:/ProjectsExternal/rema/[eval1]:1
'import
^^^^^^^

SyntaxError: Invalid or unexpected token
    at Loader.evalInstance (internal/modules/esm/loader.js:159:22)
    at new ModuleJob (internal/modules/esm/module_job.js:60:26)
    at Loader.eval (internal/modules/esm/loader.js:168:17)
    at internal/process/execution.js:49:37
    at loadESM (internal/process/esm_loader.js:68:11)
JounQin commented 3 years ago

OK, I can reproduce with Node 16 on Windows with remark-cli@9, although I still do not understand what is going on.

node --input-type module --eval 'import * as x from "remark-preset-prettier"; console.log(x)' still work.

JounQin commented 3 years ago

OK, it's a bug of unified-engine at https://github.com/unifiedjs/unified-engine/blob/8.2.0/lib/configuration.js#L298-L300

cc @wooorm

For typescript compilation, Object.defineProperty(exports, '__esModule', { value: true }); is always set for commonjs.

See https://unpkg.com/remark-preset-prettier@0.4.1/lib/cjs.js

Would you like to give it a patch fix?

JounQin commented 3 years ago

@wooorm Even after #58, it still seems unified-engine is buggy.

ReferenceError: exports is not defined in ES module scope
This file is being treated as an ES module because it has a '.js' file extension and '/Users/JounQin/Workspaces/Local/test/node_modules/remark-preset-prettier/package.json' contains "type": "module". To treat it as a CommonJS script, rename it to use the '.cjs' file extension.
    at file:///Users/JounQin/Workspaces/Local/test/node_modules/remark-preset-prettier/lib/cjs.js:3:23

loadFromAbsolutePath is called with cjs file, what means exports in package.json will not be respected.

wooorm commented 3 years ago

@JounQin I think you can solve this by using .cjs as a file extension for CommonJS.

Testing with:

$ node --input-type module --eval 'import * as x from "remark-preset-prettier"; console.log(x)'
$ node --input-type commonjs --eval 'const x = require("remark-preset-prettier"); console.log(x)'

The current 0.5.0 doesn’t work. But with explicit .mjs or .cjs extensions it does:

  "name": "remark-preset-prettier",
  "version": "0.5.0",
  // ...
  "exports": {
    "import": "./lib/index.mjs",
    "require": "./lib/cjs.cjs"
  },

Without the explicit extensions, then if you add type: module, the import will work but the require won’t. This can be solved with:

{
  "type": "module",
  // ...
  "exports": {
    "import": "./lib/index.js",
    "require": "./lib/cjs.cjs"
  },

This is because the exports map with import/require entries does not affect whether a file is actually ESM or CJS. Just that it will be chosen as such. Extensions or type: module do that!

wooorm commented 3 years ago

loadFromAbsolutePath

Correct, the last version does not respect export maps. The current version does

JounQin commented 3 years ago

@wooorm I know .cjs will work, and I'm working on it, but the behavior of unified-engine is incorrect, that should be fixed IMO.

wooorm commented 3 years ago

Which behavior and how could it be fixed?

If you mean the temporary “get ESM working” part. Yes, it’s not perfect: https://github.com/unifiedjs/unified-engine/commit/1091e327d7cf225f90c98ab9aad676efec723cfb. The behavior is the same behavior as what Babel did for their plugins. It was meant as a way to first allow plugins and config files to use import/exports. Proper export maps and ESM loading is a breaking change (e.g., you have to use import('./x.js') instead of import('./x')), and hence it landed in a new major version

JounQin commented 3 years ago

@wooorm Ideally, when ReferenceError: exports is not defined in ES module scope is thrown, it can try createRequire to load the config as cjs.


And the first question should be why the absolute resolved path is cjs instead of esm?

JounQin commented 3 years ago

And for default, it should be something like const result = await import('./x'); return result.default || result.

wooorm commented 3 years ago

And the first question should be why the absolute resolved path is cjs instead of esm?

Because the v8 engine only supports the old resolving: no export maps, only main. So it’ll find CJS. If the file extension end in .mjs, or there is an ERR_REQUIRE_ESM thrown, then it’ll try to import it: https://github.com/unifiedjs/unified-engine/commit/1091e327d7cf225f90c98ab9aad676efec723cfb.

The v9 engine switches that, and only supports the new module resolution.

For your error, ReferenceError: exports is not defined in ES module scope, That sounds like a CJS file is loaded as an ESM module. Can you explain which versions and at what commit of the code here you got that?

And for default, it should be something like const result = await import('./x'); return result.default || result.

Why? I think people expect (and should expect) that import result from './x.js' will have the exact same result when they do it in code as when they do in on the CLI. If your plugin/preset doesn‘t have an export default, then it can’t be used as such on the API. ESM is specified as such, that import x from 'y' takes the default export. Not all exports.

JounQin commented 3 years ago

@scscgit Please try remark-preset-prettier@0.5.1, it should work as expected now, although it is caused by unified-engine. I tested on my own machine with remark-cli 9/10 and node 12/14/16.

JounQin commented 3 years ago

@woorm

For your error, ReferenceError: exports is not defined in ES module scope, That sounds like a CJS file is loaded as an ESM module. Can you explain which versions and at what commit of the code here you got that?

Node 12:

image

ReferenceError: exports is not defined in ES module scope is thrown on Node 14, they are just similar.

{
  "remark-cli": "^9.0.0",
  "remark-preset-prettier": "https://pkg.csb.dev/JounQin/remark-preset-prettier/commit/2c873220/remark-preset-prettier"
}

In this version, cjs.js is used for require.

I think people expect (and should expect) that import result from './x.js' will have the exact same result when they do it in code as when they do in on the CLI.

I'm using export { plugins } before, because it will emit a better cjs file like:

Object.defineProperty(exports, '__esModule', { value: true }); // this breaks `remark-cli@9`
exports.plugins = plugins

So const preset = require('remark-preset-prettier') will work as same as import * as preset from 'remark-preset-prettier' or await import('remark-preset-prettier').

JounQin commented 3 years ago

Personally, I don't like export default because I have to type the default name manually, a named export is always preferred to myself.

wooorm commented 3 years ago

In this version, cjs.js is used for require.

Thank you for the reproduction. Yes, this file is specified as cjs, but it isn’t, because the closest package.json specifies type: module and the extension is .js. That commit doesn’t work in CJS:

$ node --input-type commonjs --eval 'const x = require("remark-preset-prettier"); console.log(x)'
node:internal/modules/cjs/loader:1150
      throw err;
      ^

Error [ERR_REQUIRE_ESM]: require() of ES Module ~/node_modules/remark-preset-prettier/lib/cjs.js from ~/[eval] not supported.
cjs.js is treated as an ES module file as it is a .js file whose nearest parent package.json contains "type": "module" which declares all .js files in that package scope as ES modules.
Instead rename cjs.js to end in .cjs, change the requiring code to use dynamic import() which is available in all CommonJS modules, or change "type": "module" to "type": "commonjs" in ~/node_modules/remark-preset-prettier/package.json to treat all .js files as CommonJS (using .mjs for all ES modules instead).

I'm using export { plugins } before, because it will emit a better cjs file like:

This is how Babel/Rollup and such indeed work, but it is not spec compliant. So it used to work in “faux-ESM” but not in actual ESM.

Personally, I don't like export default because I have to type the default name manually, a named export is always preferred to myself.

Type as in TypeScript? In that case, perhaps allowSyntheticDefaultImports in a tsconfig.json is the solution. Type as in typing on a keyboard, 🤷‍♂️ it takes like 0.3s to press the keys on a keyboard or copy/paste them, and maybe your IDE can help autocomplete it too. Regardless, I also don’t like default exports, but mostly because of how faux-ESM has treated them differently than actual ESM.

scscgit commented 3 years ago

Thanks, I confirm that using either

"remark-preset-prettier": "^0.5.1",

or

"remark-cli": "^10.0.0",

or both of them together fixes the issue, and that reverting both of them causes it to return.

I got lost in the ESM/CJS-specific details, so hopefully this knowledge of workarounds won't be necessary for library maintainers after new versions of the dependencies get released (as this probably isn't properly documented from this perspective). But just to make sure, especially related to unified if anyone else has the same issue in the future, do you know how this root cause may have retroactively affected old project installations, such that it suddenly stopped working with existing lock files and stayed broken after reinstalling node_modules? (There are often leftover changes after upgrading, so we usually need to delete both node_modules and lockfile to properly regenerate the lockfile, but I'm sure that I followed that and it got inconsistent just some time later with the same dependencies, and I couldn't have missed it after an upgrade due to lint-staged.)

JounQin commented 3 years ago

@wooorm

That commit doesn’t work in CJS:

Thank you for clarification, so .cjs extension is required in native esm package.

This is how Babel/Rollup and such indeed work, but it is not spec compliant. So it used to work in “faux-ESM” but not in actual ESM.

For this part, it seems not true to me.

// test.cjs
Object.defineProperty(exports, '__esModule', { value: true }); // with or without this line does not change the result

exports.plugins = []
// test.mjs
import * as all from './test.cjs'

import test, { plugins } from "./test.cjs";

console.log(all, test, plugins);

run node test.mjs

[Module: null prototype] {
  __esModule: true,
  default: { plugins: [] },
  plugins: []
} { plugins: [] } []

What means, exports properties will be default + named export in esm at the same time. So I still think export default should not be required.

Type as in typing on a keyboard, 🤷‍♂️ it takes like 0.3s to press the keys on a keyboard or copy/paste them, and maybe your IDE can help autocomplete it too.

I mean how to name the default exported value on import statement, actually. Sorry for my poor English for confusing.

wooorm commented 3 years ago

Yes, you are right that Node also loads CommonJS files from ESM files as such (with exports.$x as “named specifiers”, and the whole module.exports as “default”). But it doesn‘t do that for ESM in ESM:

// preset.mjs
export const plugins = []

// test.mjs
import * as all from './preset.mjs'
import test, { plugins } from './preset.mjs';

console.log(all, test, plugins);
import test, { plugins } from './preset.mjs';
       ^^^^
SyntaxError: The requested module './preset.mjs' does not provide an export named 'default'

The same when running with deno:

$ deno run test.mjs 
error: Uncaught SyntaxError: The requested module './preset.mjs' does not provide an export named 'default'
import test, { plugins } from './preset.mjs';
       ~~~~
    at <anonymous> (file:///.../example/test.mjs:3:8)
JounQin commented 3 years ago

@wooorm

But it doesn‘t do that for ESM in ESM:

Of course it doesn't, and I've never expected import test, { plugins } from './preset.mjs'; will work in native esm. But import * as test from './preset.mjs' is fine.

My point is why default must be exported to make unified-engine to get work.

And for default, it should be something like const result = await import('./x'); return result.default || result.

wooorm commented 3 years ago

My point is why default must be exported to make unified-engine to get work.

Exactly because result.default || result is different than ESM importing ESM.

Plugins can now more easily export other things (example: https://github.com/rehypejs/rehype-sanitize#api). The engine shouldn’t see those other keys as plugins or configuration options randomly.

JounQin commented 3 years ago

Exactly because result.default || result is different than ESM importing ESM.

Not sure to understand, export const plugins = {} is esm, why we cannot support it?

'default' in result ? result.default : result or something else maybe better.

It will make getting plugins more compatible.

It is not randomly but compatible IMO.

wooorm commented 3 years ago

It will make getting plugins more compatible.

I don’t want to be compatible with how Node handles CJS in ESM in the past. I want to be compatible with how ESM is used.

why we cannot support it

Because I want all plugins and presets to explicitly use export default. So that all plugins and presets can be used the same way. On both the API: import somePluginOrPreset from 'some-place', and on the CLI. By adding such handling, that would not work. If you only use the preset on the CLI, your users that want to use in on the API will get confused.

JounQin commented 3 years ago

If you only use the preset on the CLI, your users that want to use in on the API will get confused.

That makes sense to me, thanks.

Then, at least if default is not found in esm exports, unified-engine can report a more specific error message for users, that would help this original issue too.

wooorm commented 3 years ago

It doesn’t work for CJS files. Importing an empty CJS file gives:

[Module: null prototype] { default: {} }

But it does work for ESM files!

JounQin commented 3 years ago

Well, a bit more helpful error message is better.


Thanks @wooorm.