-
-
Notifications
You must be signed in to change notification settings - Fork 29
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
Add explicit typings location to support use in TypeScript ESM modules #978
Conversation
🦋 Changeset detectedLatest commit: d99169b The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
👍 I was just about to open this myself |
How did you test this change? Could you share something that I could use to verify? |
Unfortunately, this sort of thing is pretty tough to reproduce and test, because the only true test is to actually publish a package with the change and see that it works. Locally, what I did to test was to install your package into multiple downstream consumers (commonjs and module) and manually modified the Unfortunately, no amount of hackery using StackBlitz or CodeSandbox appears to allow me to make this sort of patch change to your package in-situ. In my own library, I added a script to
That doesn't seem to work on either CodeSandbox or StackBlitz, which I attribute to the fact that they aren't truly running an environment for me with those files. Even CodeSandbox's new Cloud VM mode seems not to do the trick. It's possible you could setup some crazy rube goldberg machine with The most bulletproof way to do this would be for you to issue a prerelease version ( |
That's what the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
@@ -7,7 +7,8 @@ | |||
"type": "commonjs", | |||
"exports": { | |||
"import": "./dist/index.mjs", | |||
"require": "./dist/index.js" | |||
"require": "./dist/index.js", | |||
"types": "./dist/index.d.ts" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this needs to come first: #996
The question and answer here describes the situation in detail and is exactly identical to the current situation in the
dom-accessibility-api
package.In short, TypeScript modules which are marked as
type: "module"
withmoduleResolution: "node16"
cannot consume this package because while an explicit entrypoint is specified for esm (./dist/index.mjs
), there is no corresponding typings file. In this case, TypeScript will look for such a typings file at the location./dist/index.d.mts
by default, and this file does not exist.The simplest fix for this, which is the entirety of my PR, is to provide a hint to TypeScript that types should always be resolved at
./dist/index.d.ts
regardless of whether the importer is esm or cjs. This is a one-line change which will fix the problem; I have tested this in both commonjs and esm consumers by modifying thepackage.json
in the installed version ofdom-accessibility-api
.Another, slightly more invasive approach would be to restructure your
package.json
to follow the structure they recommend in "TypeScript v4.7 Release Notes":However I think my minimal change is sufficient and works for both commonjs and esm consumers (tested in TypeScript 5.1.3 but should work for any version which supports
exports
) and will not affect older consumers which do not recognizeexports
.Since your package does not have alternate typings for any export, this is correct in any case and cannot break anyone downstream (as far as I can tell, there are no special cases where certain customers should receive different types who will be broken now).