js-temporal / temporal-polyfill

Polyfill for Temporal (under construction)
ISC License
530 stars 28 forks source link

Add types to exports/./import to support Typescript Node16 #158

Closed jonasb closed 2 years ago

jonasb commented 2 years ago

Before this Typescript would fail to resolve the types for the package when configured to use module: "Node16" (new in Typescript 4.7).

main.ts:1:26 - error TS7016: Could not find a declaration file for module '@js-temporal/polyfill'.

Repro available at https://github.com/jonasb/typescript-error-repro/tree/typescript-4.7-esm-node16-js-temporal

12wrigja commented 2 years ago

Thanks for the PR, and for creating a repro.

Looking around the community, this seems like it's unfortunately intentional (https://github.com/microsoft/TypeScript/issues/46334#issuecomment-981876926 implies that if there is an exports object that a types key is now required in the exports object instead of falling back to the top-level types). Seems similar to https://github.com/framer/motion/issues/1543.

I patched your PR and tested against a couple small test projects and they seem to compile/run just fine, so I think this is fine to merge.

everett1992 commented 2 years ago

I hit Could not find a declaration file for module '@js-temporal/polyfill' today and this change fixes the error. When will this be published to npm?

12wrigja commented 2 years ago

I've been in the middle of merging a bunch of upstream fixes, but that might take longer to release so I'll put together a patch release with this fix sometime in the next day or so.

12wrigja commented 2 years ago

This should now be published on NPM: https://www.npmjs.com/package/@js-temporal/polyfill/v/0.4.2

@everett1992 mind confirming this fixes things for you?

12wrigja commented 2 years ago

@jonasb it seems this might not have fixed things, based on the report in #177 ? Can you verify that this PR did fix things for your repro?

everett1992 commented 2 years ago

I can confirm it's fixed for my use case - using "type": "module" and native esm. #177 sounds like an issue with commonjs require..