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

Fix dual emit declaration files #9

Merged
merged 3 commits into from
Jun 28, 2023

Conversation

andrewbranch
Copy link
Contributor

tsup just published egoist/tsup#934, which fixes a module format agreement issue that you inherited from its output. Details in the tsup PR, but:

Before

🎭 Import resolved to a CommonJS type declaration file, but an ESM JavaScript file.
https://github.com/arethetypeswrong/arethetypeswrong.github.io/blob/main/docs/problems/FalseCJS.md

🐛 Import resolved to types through a conditional package.json export, but only after
failing to resolve through an earlier condition. This behavior is a TypeScript bug. It
may misrepresent the runtime behavior of this import and should not be relied upon.
https://github.com/arethetypeswrong/arethetypeswrong.github.io/blob/main/docs/problems/FallbackCondition.md


┌────────────────────┬───────────────────────────────────┬───────────────────────────────────┐
│                    │ "@total-typescript/shoehorn"      │ "@total-typescript/shoehorn/pack… │
├────────────────────┼───────────────────────────────────┼───────────────────────────────────┤
│ node10             │ 🟢                                │ 🟢 (JSON)                         │
├────────────────────┼───────────────────────────────────┼───────────────────────────────────┤
│ node16 (from CJS)  │ 🟢 (CJS)                          │ 🟢 (JSON)                         │
├────────────────────┼───────────────────────────────────┼───────────────────────────────────┤
│ node16 (from ESM)  │ 🎭 Masquerading as CJS            │ 🟢 (JSON)                         │
│                    │ 🐛 Used fallback condition        │                                   │
├────────────────────┼───────────────────────────────────┼───────────────────────────────────┤
│ bundler            │ 🐛 Used fallback condition        │ 🟢 (JSON)                         │
└────────────────────┴───────────────────────────────────┴───────────────────────────────────┘

After

❯ npm run attw                          

> @total-typescript/shoehorn@0.1.1 attw
> attw --pack


 No problems found 🌟


┌────────────────────┬───────────────────────────────────┬───────────────────────────────────┐
│                    │ "@total-typescript/shoehorn"      │ "@total-typescript/shoehorn/pack… │
├────────────────────┼───────────────────────────────────┼───────────────────────────────────┤
│ node10             │ 🟢                                │ 🟢 (JSON)                         │
├────────────────────┼───────────────────────────────────┼───────────────────────────────────┤
│ node16 (from CJS)  │ 🟢 (CJS)                          │ 🟢 (JSON)                         │
├────────────────────┼───────────────────────────────────┼───────────────────────────────────┤
│ node16 (from ESM)  │ 🟢 (ESM)                          │ 🟢 (JSON)                         │
├────────────────────┼───────────────────────────────────┼───────────────────────────────────┤
│ bundler            │ 🟢                                │ 🟢 (JSON)                         │
└────────────────────┴───────────────────────────────────┴───────────────────────────────────┘

I’ve also taken the liberty of including this check as part of your release process, since unlike tsc, tsup does not actually check that the output is actually valid in all these scenarios.

Comment on lines 10 to +11
"import": "./dist/index.mjs",
"require": "./dist/index.js",
"types": "./dist/index.d.ts"
"require": "./dist/index.js"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We’re letting extension substitution do the work here, but if you want to be explicit about specifying types, this has the same outcome:

{
  "exports": {
    ".": {
      "import": {
        "types": "./dist/index.d.mts",
        "default": "./dist/index.mjs"
      },
      "require": {
        "types": "./dist/index.d.ts",
        "default": "./dist/index.js"
      }
    }
  }
}
         

@@ -18,15 +17,17 @@
"dev": "vitest",
"build": "tsup src/index.ts --format cjs,esm --dts",
"lint": "tsc",
"release": "turbo build lint test && changeset publish"
"attw": "attw --pack",
"release": "turbo build lint test attw && changeset publish"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'll need to add attw to turbo.json to get this working I think.

What outputs does attw --pack have?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just an exit code (and stdout whether it succeeds or fails, but only interesting in CI if you need to look at why it failed). It runs npm pack but deletes the file after checking it.

@mattpocock
Copy link
Collaborator

Wow, pal - thanks so much for making this change on this repo.

Small changes needed to turbo.json, outlined in comment.

@andrewbranch
Copy link
Contributor Author

Sure thing! I discovered recently that you were a tsup user, so I figured something like this would be a good way to prove out the tsup change I made on something real, and be able to link to it as an example of how to take advantage of the new output, since a small package.json change is necessary.

@mattpocock mattpocock merged commit 99109c0 into total-typescript:main Jun 28, 2023
@mattpocock
Copy link
Collaborator

Looks great, thanks pal!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants