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.

Sign up for GitHub

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

Jump to bottom

Cannot import correctly under TypeScript Node 16 module resolution #992

Closed
1 task done
Jack-Works opened this issue Nov 5, 2022 · 13 comments
Closed
1 task done

Cannot import correctly under TypeScript Node 16 module resolution #992

Jack-Works opened this issue Nov 5, 2022 · 13 comments

Comments

@Jack-Works
Copy link

🐛 Bug Report

Under TypeScript Node 16 module resolution (has type: module in the package.json and moduleResolution: Node16 in tsconfig.json), this package cannot be imported correctly.

image

TypeScript thinks this package is a CommonJS package and therefore provides the synthetic export for the CommonJS module.

To Reproduce

import produce from 'immer'

produce(() => {}, {})
// TS: This expression is not callable.

produce.default(() => {}, {})
// TS ok, but runtime error: produce.default is not a function

package.json

{ "type": "module" }

tsconfig.json

{
    "compilerOptions": {
        "moduleResolution": "Node16"
    }
}

Observed behavior

Cannot be imported.

Expected behavior

Imported correctly.

Environment

  • Immer version:
  • I filed this report against the latest version of Immer
@mweststrate
Copy link
Collaborator

Alternatively, import {produce} from "immer"

@joaolpgomes87 joaolpgomes87 mentioned this issue Jan 3, 2023
Closed
@markerikson
Copy link
Contributor

@mweststrate fyi, per https://arethetypeswrong.github.io/?p=immer%409.0.21 the package settings probably need some tweaks.

I've been working on trying to get the Redux packages fixed up to pass, and have been setting up a bunch of CI checks to help verify compatibility. One of them is a CLI script + CI job that runs that arethetypeswrong tool and prints out a table similar to the website report:

I'm about to take a stab at updating redux-thunk, redux, and then @reduxjs/toolkit with better package configs, probably based on reduxjs/redux-toolkit#3211 .

Assuming I get these working, might be worth using them as the basis for Immer 10's packaging too.

@mweststrate
Copy link
Collaborator

mweststrate commented Apr 3, 2023 via email

@markerikson
Copy link
Contributor

Yeah, I actually just things seemingly working for all three of those packages?

Here's a summary comment in the PR that updated RTK, describing the package settings I settled on:

reduxjs/redux-toolkit#3318 (comment)

@mweststrate
Copy link
Collaborator

mweststrate commented Apr 3, 2023 via email

@markerikson
Copy link
Contributor

@mweststrate just looking at that report, it looks like a more serious actual "error".

What I ended up with is attw reporting a "FalseCJS" status when used with Node16 resolution. In other words, it found the ESM build artifacts fine, but since the artifact has a .mjs extension and the types have a .d.ts extension, it thinks they're "types that match a CJS artifact". Andrew Branch has been pointing out that in theory you should have separate typedefs for CJS and ESM artifacts because there could be subtle differences in the exports.

I think, and I haven't gotten confirmation on this, that the "FalseCJS" status is a relatively ignorable warning that shouldn't matter in practice.

The attw report for this Immer beta, on the other hand, looks like there's a real (?) problem.

You've currently got:

  "main": "dist/index.js",
  "module": "dist/immer.esm.js",
  "exports": {
    ".": {
      "types": "./dist/immer.d.ts",
      "import": "./dist/immer.esm.js",
      "require": "./dist/index.js"
    }
  },

The attw errors say that for node16 resolution in CJS mode:

  • ESM (dynamic import only): Imports of the package resolved to ES modules from a CJS importing module. CJS modules in Node will only be able to access this entrypoint with a dynamic import.
  • Unexpected CJS syntax: The implementation resolved at the package uses CJS syntax, but the detected module kind is ESM. This will be an error in Node (and potentially other runtimes and bundlers). The module kind was decided based on the nearest package.json’s "type" field.

I'm not immediately sure why it's reporting those problems tbh. It seems to have found /node_modules/immer/dist/immer.d.ts as the types and /node_modules/immer/dist/index.js as the implementation, both of which seem to be intended and expected for CJS mode.

Oh. It might be because you have type: "module", which forces all .js files to be treated as ESM? Andarist advised me to drop that, and rely on the combination of .mjs/.cjs extensions and exports conditions. I had always assumed based on my reading that type: "module" was critical for "full ESM" support, but it's not - it really just controls how Node interprets .js files in the folder. I don't like the .mjs/.cjs extensions, but those do a much more precise job of controlling Node's interpretation.

I'll go ahead and try bumping RTK's Immer dep to the beta now and see how it works, but based on what I think I'm seeing here, my thoughts are:

  • That concern is only in TS's node16 resolution mode, which isn't widely used yet. It ought to be fixed before 10.0 goes final, but shouldn't block this first beta
  • I think you should probably consider dropping type: "module" from the package and using .mjs/.cjs extensions for the build artifacts

@markerikson
Copy link
Contributor

Yeah, sure enough, one of the Node example projects I set up for RTK's CI pipeline fails with this Immer beta:

/home/runner/work/redux-toolkit/redux-toolkit/examples/publish-ci/node-esm/node_modules/@reduxjs/toolkit/dist/cjs/redux-toolkit.development.cjs:67
var import_immer5 = require("immer");
^

Error [ERR_REQUIRE_ESM]: require() of ES Module /home/runner/work/redux-toolkit/redux-toolkit/examples/publish-ci/node-esm/node_modules/immer/dist/index.js from /home/runner/work/redux-toolkit/redux-toolkit/examples/publish-ci/node-esm/node_modules/@reduxjs/toolkit/dist/cjs/redux-toolkit.development.cjs not supported.
index.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 index.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 /home/runner/work/redux-toolkit/redux-toolkit/examples/publish-ci/node-esm/node_modules/immer/package.json to treat all .js files as CommonJS (using .mjs for all ES modules instead).

@mweststrate
Copy link
Collaborator

Does beta.3 fare any better? I only dropped the type definition. https://arethetypeswrong.github.io/?p=immer%4010.0.0-beta.3 still reports a problem, but I don't get why since the main is CJS, and module is ESM, so that looks pretty correct to me intuitively?

@mweststrate
Copy link
Collaborator

This looks better: https://arethetypeswrong.github.io/?p=immer%4010.0.0-beta.4. Don't look at the sources, yikes. Hopefully doesn't break sourcemaps

mweststrate added a commit that referenced this issue Apr 3, 2023
@mweststrate
Dropped module:type per #992 (comment)
bbd7fb2
@markerikson
Copy link
Contributor

Yeah! Now it's "just" got the same "FalseCJS" issue that I'm seeing over on the RTK side :)

FYI Andrew Branch and I were just discussing that particular problem over in this thread:

arethetypeswrong/arethetypeswrong.github.io#21

His latest comment was:

Ultimately, running tsc twice on .js files under nodenext, swapping the package.json type between runs, is the most foolproof way to generate an ESM/CJS pair of declarations.

but there's no easy way to do this atm.

@markerikson
Copy link
Contributor

markerikson commented Apr 3, 2023 •
edited
Loading

Also FYI:

I see that as of beta.4, you have: "module": "dist/immer.mjs". Unfortunately, yesterday I found out that Webpack 4 doesn't like it when the module field points to a .mjs file :( So I'm currently making a separate artifact that still has a .js extension just for use with that field.

For redux and redux-thunk, I'm just copying the same .mjs file over and changing the extension.

For RTK, where we use a bunch of optional chaining, I'm generating a fallback build artifact that has a .js extension and also targets ES2017, since Webpack 4's parser can't handle that either.

I'm about ready to push out another RTK alpha that includes Immer 10 beta. You have time now-ish to try adjusting that module artifact? If so I'll hold off and do something else while you get that up, otherwise I can go ahead and publish + announce the RTK alpha and have it use Immer 10.0-beta.4

@markerikson
Copy link
Contributor

markerikson commented Apr 3, 2023 •
edited
Loading

Huh. Somewhat surprisingly, beta.4 seems to have built okay with our cra4 test job. Maybe it's because Immer has its own bundle that's a .mjs file, but doesn't import from other bundles the way RTK imports from redux?

I'm gonna go ahead and push out an RTK alpha that uses beta.4 since this seems to be working-ish for now!

@markerikson markerikson mentioned this issue Apr 3, 2023
Merged
@Jack-Works
Copy link
Author

this is fixed in immer v10

@FengShangWuQi FengShangWuQi mentioned this issue Jun 16, 2023
Closed
@BlueEagle BlueEagle mentioned this issue Aug 31, 2023
Open
@FengShangWuQi FengShangWuQi mentioned this issue Nov 4, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@markerikson @mweststrate @Jack-Works