-
Notifications
You must be signed in to change notification settings - Fork 602
js: Fix resolution of TypeScript .d.ts definitions #997
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
Conversation
Amazingly, it looks like the import must be from |
|
One more thing I'm confused by, besides https://github.com/donmccurdy/glTF-Transform/actions/runs/20049818795 |
|
Very likely conditional on the resolution mode, yes. I'm using Before switching to the subpath imports the build passed in CI but raised errors locally in my editor, which uses typescript-language-server:
Related: donmccurdy/glTF-Transform#1780 |
|
Testing against a clean TypeScript project and a modern build tool (tried: vite, tsup, pkgroll, tsc), including with
|
|
I think what's left in the PR should be ready, for the moduleResolution=NodeNext fix! |
|
Yeah the nodenext problem seems straightforward; I had a local test project for TypeScript surface which works as is with moduleResolution=bundler but breaks when moduleResolution is changed to nodenext. Adding the extensions fixes that; in my project, I don't see any other errors (I'm using a mix of I can merge this and publish a 1.0.1 with the fix tomorrow. |
|
As for Do I understand it correctly that if I publish 1.0.1 with just the |
|
I don't expect any further issues with the 'meshoptimizer' root path import after this fix, correct! For the
... it looks like my build tool doesn't fully support I might try to (1) publish a glTF Transform v4.3 release in the next few weeks, and then follow up with a v4.4 release updating the build to support package.json#exports and updating to meshoptimizer v1.0.0. At that point it should be compatible with the current meshoptimizer v1.0.0 package. If you wanted to wait on publishing v1.0.1 until then, as no one else has reported the moduleResolution=NodeNext compatibility issue yet, that could be a good option too. |
38c18e8 to
f1654f7
Compare
|
It probably makes sense to publish a fix anyway as someone will run into this sooner or later, and it's easier to do this early so that no branching is necessary to pick up just the relevant fixes. I've bumped the package version here, will merge & publish later. |
|
I've published 1.0.1 with these changes; thanks! |
|
As a sanity check, there are tools that help discover and diagnose configuration since it is very complicated. For types: https://arethetypeswrong.github.io/?p=meshoptimizer%401.0.1 Everything looks good here. |
For TypeScript projects that have adopted certain newer configurations (
"moduleResolution": "nodenext"in tsconfig.json is probably the most common), it's necessary to include .js extensions on import paths within.d.tsfiles. (Despite the fact that we're referring to other.d.tsfiles, yeah...). Fortunately, this is backwards-compatible with older TypeScript configurations.The addition of theReverted this part.package.json#typesVersionsfixes a build issue for glTF Transform after I switched over to importing from subpaths likemeshoptimizer/decoder, but I'm still trying to figure out why — I don't typically use the "typesVersions" field so this was unexpected to me. Duplicating the .d.ts paths under bothexportsandtypesVersionsshouldn't be necessary.I'll mark this as a draft until I understand that better. I suspect this is not a common issue for JS/TS projects, so no particular rush on reviewing or publishing a patch. :)
Background: