jprichardson / node-fs-extra

Node.js: extra methods for the fs object like copy(), remove(), mkdirs()
MIT License
9.44k stars 776 forks source link

Cannot find module 'fs-extra/esm' or its corresponding type declarations.ts(2307) #979

Closed mtdvlpr closed 1 year ago

mtdvlpr commented 1 year ago

Both importing from fs-extra and fs-extra/esm doesn't work anymore in v11.0.0. I tried the following lines in my fs.ts file:

import { ensureDirSync, ensureFileSync, removeSync } from 'fs-extra'
import { ensureDirSync, ensureFileSync, removeSync } from 'fs-extra/esm'

The top line does work in v10.1.0. Am I doing something wrong?

RyanZim commented 1 year ago

The fact that the first line worked in v10.1.0 indicates your typescript transpiler is doing some kind of extra compatibility layer here, which somehow was broken in v11. I'm not a typescript guy, so I can't say offhand what's gone wrong here. Are you running the latest version of TS? If all else fails, a minimal example repo will probably be useful here.

caugner commented 1 year ago

@RyanZim I just had a quick skim through https://www.sensedeep.com/blog/posts/2021/how-to-create-single-source-npm-module.html and found this:

One more wrinkle, TypeScript does not (yet) behave with exports. So you need to include the legacy module and main properties for TypeScript. The main property points to the CJS entry point and the module property points to the ESM entry.

Based on that, this should work:

{
  "main": "./lib/index.js",
  "module": "./lib/esm.mjs",
  "exports": {
    "import": "./lib/esm.mjs",
    "require": "./lib/index.js"
  }
}

In other words, removing main from package.json may have removed TypeScript support.

Update: Just double checked, and those changes resolve the issue with TypeScript.

(Background: https://github.com/mdn/yari/pull/7697)

RyanZim commented 1 year ago

This is fixed in TypeScript v4.7: https://devblogs.microsoft.com/typescript/announcing-typescript-4-7/#package-json-exports-imports-and-self-referencing

RyanZim commented 1 year ago

Furthermore, your proposed change would be a breaking change; since the ESM version is not compatible with the regular version of fs-extra, we can't just ship it to all import users.

caugner commented 1 year ago

This is fixed in TypeScript v4.7

We're on TypeScript 4.9.

Furthermore, your proposed change would be a breaking change

Not sure why you just close my draft PR, but okay. 🤷

caugner commented 1 year ago

FWIW I only found two packages among a long list of dependencies that we're using that export as ./esm (flatted and ts-node), and both also define main and module in their package.json.

PS: The polite way would have been to add a review to my PR and ask me to revert the changes to the exports field. Just saying.

RyanZim commented 1 year ago

You may have read it already, but just to make it abundantly clear, fs-extra/esm is not just an ESM version of fs-extra: https://github.com/jprichardson/node-fs-extra#esm

I don't use TS, so perhaps I'm misunderstanding, but from the TS v4.7 release notes, it looks like they added support for exports. What am I missing?

Not sure why you just close my draft PR, but okay. shrug

Sorry, didn't notice it was a draft PR. Didn't mean to offend, feel free to open a new one.

Is module TS-specific? I'm not familiar with it, and I can't find anything about it in Node.js docs.

caugner commented 1 year ago

I don't use TS, so perhaps I'm misunderstanding, but from the TS v4.7 release notes, it looks like they added support for exports. What am I missing?

Based on a quick look through the package.jsons in our dependency tree, you're using the exports field differently that most packages. For most packages, the value for . inside exports is an object with keys import/types/default.

For example: https://github.com/WebReflection/flatted/blob/2ffe0c8f513b86d51159460a1a91561df9ba60fe/package.json#L54-L62

mtdvlpr commented 1 year ago

Is module TS-specific? I'm not familiar with it, and I can't find anything about it in Node.js docs.

Bundler tools, like Rollup and Webpack use it. See this StackOverflow post for more information.

caugner commented 1 year ago

@RyanZim https://github.com/jprichardson/node-fs-extra/pull/981 is now the minimal change to resolve the issue, without any other side-effects. 😉

caugner commented 1 year ago

The fact that the first line worked in v10.1.0 indicates your typescript transpiler is doing some kind of extra compatibility layer here, which somehow was broken in v11.

FWIW Note that using import in TypeScript doesn't imply using ESM. It depends on the moduleResolution compiler option (we use "moduleResolution": "node", for instance).

edit: And that's the important bit from that TypeScript blog post cited above:

TypeScript 4.7 adds this functionality with two new module settings: node16 and nodenext.

RyanZim commented 1 year ago

Based on a quick look through the package.jsons in our dependency tree, you're using the exports field differently that most packages. For most packages, the value for . inside exports is an object with keys import/types/default.

Yeah, this is shorthand notation: https://nodejs.org/api/packages.html#exports-sugar I'm curious if changing it to an object with only default would also fix TS support (not sure I want to do that as the actual fix, as it's non-intuitive). Based on this, it seems this would be a potential bug that TS should fix?

btakita commented 1 year ago

This is still a problem as of v11.1.1 & Typescript 5.1.0-dev.20230417.

Can't use this library b/c of this issue.

RyanZim commented 1 year ago

@btakita Importing fs-extra should work fine on fs-extra v11.1.0+, fs-extra/esm is the bit that doesn't work.

litingyes commented 1 year ago

env: "typescript": "^5.0.4",

The code works fine with esm, but the type error cannot be eliminated

Screenshot 2023-06-18 at 10 13 20

and if i use import { readJSONSync } from 'fs-extra' with

  "exports": {
    ".": {
      "require": "./lib/index.js",
      "import": "./lib/esm.mjs"
    }
  },

, there seems to be no problem

RyanZim commented 1 year ago

fs-extra/esm is not a drop-in replacement for fs-extra; fs-extra/esm is not something you should be using in a typescript environment.