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

typescript 5.2 broke the ability to build hybrid node packages #55705

Closed
isaacs opened this issue Sep 11, 2023 · 8 comments
Closed

typescript 5.2 broke the ability to build hybrid node packages #55705

isaacs opened this issue Sep 11, 2023 · 8 comments
Labels
Duplicate An existing issue was already created

Comments

@isaacs
Copy link

isaacs commented Sep 11, 2023

πŸ”Ž Search Terms

hybrid
commonjs
esm
module
moduleResolution
package.json

πŸ•— Version & Regression Information

  • This changed between versions 5.1 and 5.2

⏯ Playground Link

No response

πŸ’» Code

src/index.ts

export const foo = 'bar'

tsconfig.json

{
  "include": ["src/*.ts"],
  "compilerOptions": {
    "declaration": true,
    "declarationMap": true,
    "inlineSources": true,
    "esModuleInterop": true,
    "forceConsistentCasingInFileNames": true,
    "resolveJsonModule": true,
    "skipLibCheck": true,
    "sourceMap": true,
    "strict": true,
    "target": "es2022",
    "moduleResolution": "nodenext",
    "module": "nodenext"
  }
}

tsconfig/cjs.json

  "compilerOptions": {
    "module": "commonjs",
    "moduleResolution": "node16",
    "outDir": "../dist/cjs"
  }

tsconfig/esm.json

  "compilerOptions": {
    "outDir": "../dist/mjs"
  }

package.json

{
  "name": "foo",
  "version": "1.2.3",
  "type": "module",
  "exports": {
    ".": {
      "import": {
        "types": "./dist/mjs/index.d.ts",
        "default": "./dist/mjs/index.js"
      },
      "require": {
        "types": "./dist/cjs/index.d.ts",
        "default": "./dist/cjs/index.js"
      }
    }
  },
  "scripts": {
    "prepare": "tsc -p tsconfig/cjs.json && tsc -p tsconfig/esm.json && bash scripts/fixup.sh"
  }
}

scripts/fixup.sh

#!/usr/bin/env bash

cat >dist/cjs/package.json <<!EOF
{
  "type": "commonjs"
}
!EOF

cat >dist/mjs/package.json <<!EOF
{
  "type": "module"
}
!EOF

πŸ™ Actual behavior

tsconfig/cjs.json:4:15 - error TS5110: Option 'module' must be set to 'Node16' when option 'moduleResolution' is set to 'Node16'.

πŸ™‚ Expected behavior

Should create a CommonJS module at ./dist/cjs/index.js.

Should create an ESM module at ./dist/mjs/index.js

Should not emit errors.


I also tried setting the module to node16 in the cjs.json, but then it emits ESM for the CJS build.

If I remove "type": "module" from the root package.json (first of all, that's a pain, because my tests are ESM, but w/e), then it emits CommonJS for both.

If it's going to force module to match moduleResolution, and infer the type from the package.json file, then it should infer the type from the package.json file that is closest to the build location, not the source location, because that is where Node will get the type from.

Better yet, it should just allow CommonJS even if the package.json is "type": "module" and has a "require" export point.

Or just trust that the author of the config file knows what they're doing, and obey the explicitly stated config. If someone writes "module": "commonjs", then TSC should emit a commonjs module, regardless of what the package.json's type is.

Additional information about the issue

The only workaround I can see right now is to have my build script edit the root package.json (or write a package.json temporarily in ./src) in between each build, and that just feels terribly brittle.

@fatcerberus
Copy link

See #54567.

@isaacs
Copy link
Author

isaacs commented Sep 11, 2023

@fatcerberus That faq doesn't cover the use case described here.

The way I've been doing hybrid modules (as described in this issue) doesn't exhibit the problems described in that faq. For example: https://arethetypeswrong.github.io/?p=is-actual-promise%401.0.0 (I have many others. See also rimraf, mkdirp, glob, lru-cache, minimatch, minipass, isexe, and almost everything in the @tapjs namespace.)

My recourse here seems to be that I just need to edit the package.json file between builds, which is pretty unfortunate.

Packages can contain both commonjs and esm. That's what the package.json exports field is for. If tsc is building to a location that is the target of a require export, then emitting esm is always going to be broken, regardless of the type field in package.json.

@fatcerberus
Copy link

fatcerberus commented Sep 11, 2023

it should infer the type from the package.json file that is closest to the build location, not the source location, because that is where Node will get the type from

That would be #54546, which was abandoned due to conflicting design constraints IIRC. (I also just noticed that you did indeed comment on that issue)

You might be interested in following this issue, however: #54593

@andrewbranch andrewbranch added the Duplicate An existing issue was already created label Sep 11, 2023
@andrewbranch
Copy link
Member

typescript 5.2 broke the ability to build hybrid node packages

The way this was being done prior to 5.1 was a bug, not a feature. It was super unsafe and was internally inconsistent between module resolution, module checking, and emit. There are already many third-party solutions for unsafely doing dual-emit. tsc is currently the only good type checker for TypeScript and a pretty poor emitter, so it doesn’t make a lot of sense to keep a mode around that does incorrect type checking while emitting slower than any popular third-party emitter.

I just need to edit the package.json file between builds, which is pretty unfortunate.

Unfortunate for DX, but you’re actually getting correct type checking when you do that, assuming you put a package.json with the appropriate "type" field in the output directory too, as you’re doing in your packages.

If tsc is building to a location that is the target of a require export, then emitting esm is always going to be broken, regardless of the type field in package.json.

I mean, yes and no. The code being emitted is not broken, but the exports configuration is likely a mistake. This isn’t something tsc can fix, thoughβ€”simply swapping the emit format to CJS is not valid either, since according to the package.json files tsc knows about, the output file with a .js extension is unambiguously going to be interpreted as ESM. On the flip side, pointing an import conditional export to a CommonJS file is totally legal and there may be good reasons to do it. The same .js file can sometimes be the target of both a require and an import conditional export. TLDR, the exports configuration is not and cannot be a reliable signal of the intended output format of emitted files.

I think something like #54546 is probably still on the table. Also somewhat related is #55221, where I’m considering allowing for finer grained control of various pieces of module infrastructure, including how module format is detected, possibly including values that override any package.json detection, which would suit your use case.

@isaacs
Copy link
Author

isaacs commented Sep 11, 2023

If tsc is building to a location that is the target of a require export, then emitting esm is always going to be broken, regardless of the type field in package.json.

I mean, yes and no. The code being emitted is not broken, but the exports configuration is likely a mistake. This isn’t something tsc can fix, thoughβ€”simply swapping the emit format to CJS is not valid either, since according to the package.json files tsc knows about, the output file with a .js extension is unambiguously going to be interpreted as ESM. On the flip side, pointing an import conditional export to a CommonJS file is totally legal and there may be good reasons to do it. The same .js file can sometimes be the target of both a require and an import conditional export. TLDR, the exports configuration is not and cannot be a reliable signal of the intended output format of emitted files.

Yes, something can be both import and require if it's commonjs, that's true.

Also somewhat related is #55221, where I’m considering allowing for finer grained control of various pieces of module infrastructure, including how module format is detected, possibly including values that override any package.json detection, which would suit your use case.

Awesome.

Yes, I would love a --moduleType commonjs #NoReallyIMeanIt option that would override any other type of inference from package.json, resolution style, etc.

That's sort of what --module was in 5.1, when they were allowed to be "mismatched" in this way. I get that people found it confusing, but it's simply not the case that this was always necessarily a bug, as implied in the FAQ. It wasn't "luck", it was just reading the docs carefully, and then configuring my build system to do what I wanted.

It seems like the solution for ts 5.2 at least is "edit package.json between builds", and I'm most of the way through writing a tool that'll just do all this for me automatically anyway. Feel free to close this, but consider it a vote in favor of "please give me back my footgun, I was using it, and my feet are perfectly safe, I promise."

@andrewbranch
Copy link
Member

That's sort of what --module was in 5.1, when they were allowed to be "mismatched" in this way. I get that people found it confusing, but it's simply not the case that this was always necessarily a bug, as implied in the FAQ. It wasn't "luck", it was just reading the docs carefully, and then configuring my build system to do what I wanted.

No, it really was materially different from what you get by modifying your package.json "type". The inaccuracies don’t always manifest, but if you had dependencies that used exports in a way that exposes a different API to ESM vs CJS (whether significantly different or just default export shenanigans), no amount of vigilance on your end would have convinced TypeScript to resolve to the correct dependency typings when you swapped your module value.

I'm most of the way through writing a tool that'll just do all this for me automatically anyway

I strongly considered doing this myself. Are you considering packaging that tool as a standalone dependency? I would love to have more good workarounds I can recommend to people before whatever comes out of #55221.

consider it a vote in favor of "please give me back my footgun, I was using it, and my feet are perfectly safe, I promise."

The new footgun will be much more accurate πŸ˜„

@fatcerberus
Copy link

The new footgun will be much more accurate πŸ˜„

I think I’d prefer my footguns to be inaccurate if I’m actually firing them. πŸ˜‰

@isaacs
Copy link
Author

isaacs commented Sep 12, 2023

I strongly considered doing this myself. Are you considering packaging that tool as a standalone dependency? I would love to have more good workarounds I can recommend to people before whatever comes out of #55221.

Of course! Not quite ready for prime time, still working the kinks out of it by porting tapjs to it, but should be good to go in the next few days or so: https://github.com/isaacs/tshy

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Duplicate An existing issue was already created
Projects
None yet
Development

No branches or pull requests

3 participants