Make astro package play nice with node16 module resolution #4182
Conversation
Projects using node16 module resolution in typescript uses the new exports and imports configuration from typescript to find definition files. This mirrors how nodejs resolves the files. If a package contains an exports map in the package.json, typescript will ignore the "types" field (not sure how it plays with typesVersions). This moves the typings hirearchy of definition files into the same hierarchy that astro produces output files in, so that typescript can discover them. Fixes: withastro#4172
🦋 Changeset detectedLatest commit: 0714a8e The changes in this PR will be included in the next version bump. This PR includes changesets to release 14 packages
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 |
github-actions
bot
added
the
pkg: astro
|
This all makes sense to me! Thanks for the investigation and the thorough PR. As Nate said in the issue I’d also want a +1 from out resident TS expert @Princesseuh before merging. |
packages/astro/package.json
Outdated
| ] | ||
| } | ||
| }, | ||
| "exports": { | ||
| ".": "./astro.js", | ||
| ".": {"default":"./astro.js", "types": "./dist/@types/astro.d.ts"}, |
There was a problem hiding this comment.
Note: the types condition should come before default. (typescript docs)
There was a problem hiding this comment.
This is JSON, the order is arbitrary. I see the example in the docs has it before, but that's just an example, and nowhere in the text I read that it should come before (not as far as I can understand at least) - if I'm reading this wrong, please point out where I'm misreading.
Also do note that this is actually working, as I'm running locally with a pnpm patch to make astro work for me.
[Edit]
Note; I'm not opposed to re-order. If you want it in the other order that's fine (and I don't really care). If however typescript requires the types-field to be first, I would like to know, so I know not to make the error again. If it does not, I would also like to know that I can just order them however I like for other projects.
There was a problem hiding this comment.
It's shown in this code example in the linked docs:
and in the announcement post:
Disclaimer: I've created publint, so I'm picky with this 🙂
There was a problem hiding this comment.
It doesn't really matter in this case, but Node's documentation also mention that default should always be last:
There was a problem hiding this comment.
I love being picky as long as there's clear rules, and in this case there seems to be. I think it's a bad idea to have order-dependent JSON, so I'd argue node made a bad decision here, but it's what we got to live with, I guess. I'll update the pr when I'm back at my computer.
There was a problem hiding this comment.
Oh wow, I didn't know about these rules! That's super annoying, but yeah it would be great to make sure we're following them exactly.
There was a problem hiding this comment.
I have updated the order.
There was a problem hiding this comment.
Thank you for making this PR, support for moduleResolution: 'node16' is definitely something we want in Astro! In addition to the comment about the order of the types, the path to the types should be updated in astro-jsx.d.ts.
Additionally, an important note about this: While this unblock usage inside .ts(x) files, moduleResolution: 'node16' is currently unsupported in our language server so inside .astro file, even if you set it to node16, node will be instead forced on.
Hopefully this is not a dealbreaker for you to use Astro, we're definitely planning to support node16 as soon as possible, but we're currently focused on other tasks.
Apart from that, it looks good to me. It's a bit hard to tell on such a large change, but I don't see any reasons it would break anything. However, just to be sure we don't break everything right before the release of 1.0 (tomorrow!), we'll wait after the release to merge this in, hope that's okay.
|
I'm working around the issue of astro-files not caring about |
|
This is marked as being a minor release, so it can go out the next time we do one of those. |
Projects using node16 module resolution in typescript uses the new
exports and imports configuration from typescript to find definition
files. This mirrors how nodejs resolves the files. If a package contains
an exports map in the package.json, typescript will ignore the "types"
field (not sure how it plays with typesVersions). This moves the typings
hirearchy of definition files into the same hierarchy that astro
produces output files in, so that typescript can discover them.
Changes
Projects using node16 module resolution in typescript uses the new
exports and imports configuration from typescript to find definition
files. This mirrors how nodejs resolves the files. If a package contains
an exports map in the package.json, typescript will ignore the "types"
field (not sure how it plays with typesVersions). This moves the typings
hirearchy of definition files into the same hierarchy that astro
produces output files in, so that typescript can discover them.
Testing
I've used
pnpm linklocally to doimport type { AstroIntegration } from 'astro';- it does not work if I just use theastropackage from NPM.Docs
This should only be visible in that people who use the new module resolution can now get typings - and also, the file hierarchy of the dist folder in the package is modified in that type-declarations now live next to their js counterparts.
Fixes: #4172