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: add type declarations to exports field #6307

Merged
merged 6 commits into from
Jun 2, 2022

Conversation

andipaetzold
Copy link
Contributor

@andipaetzold andipaetzold commented May 27, 2022

Adds paths of type definitions to each package.json's exports

I am the least confident about the changes to @firebase/auth due to the many different exports. I hope I got it right.

Discussion

closes #6300

context: https://devblogs.microsoft.com/typescript/announcing-typescript-4-7/#esm-nodejs

Testing

I am not quite sure how to test these paths. Ideally, all paths within the exports fields should be tested to avoid referencing a missing file. Maybe a new global test that checks all packages could do the job?

@changeset-bot
Copy link

changeset-bot bot commented May 27, 2022

🦋 Changeset detected

Latest commit: 19adc31

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 31 packages
Name Type
@firebase/analytics-compat Patch
@firebase/analytics Patch
@firebase/app-check-compat Patch
@firebase/app-check Patch
@firebase/app-compat Patch
@firebase/app Patch
@firebase/auth-compat Patch
@firebase/auth Patch
@firebase/component Patch
@firebase/database-compat Patch
@firebase/database Patch
firebase Patch
@firebase/firestore-compat Patch
@firebase/firestore Patch
@firebase/functions-compat Patch
@firebase/functions Patch
@firebase/installations-compat Patch
@firebase/installations Patch
@firebase/logger Patch
@firebase/messaging-compat Patch
@firebase/messaging Patch
@firebase/performance-compat Patch
@firebase/performance Patch
@firebase/remote-config-compat Patch
@firebase/remote-config Patch
@firebase/storage-compat Patch
@firebase/storage Patch
@firebase/template Patch
@firebase/util Patch
@firebase/webchannel-wrapper Patch
@firebase/database-types Patch

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

@andipaetzold andipaetzold changed the title fix: add type definitions to exports field fix: add type declarations to exports field May 27, 2022
@hsubox76
Copy link
Contributor

Thank you very much for putting together this PR!

I read through the Typescript documentation for this change (https://devblogs.microsoft.com/typescript/announcing-typescript-4-7/#package-json-exports-imports-and-self-referencing) and if I'm understanding it correctly, the types field is only needed when the typings file doesn't have the same name (and isn't in the same directory) as the entry point file. For example, if the entry point is dist/index.js then TypeScript will automatically expect the typings file to be dist/index.d.ts.

So the only files that should be broken are ones with non-matching names, mostly files with public in them, which are named that because they have gone through an additional step to remove non-public types. I don't think we need a types field in cases where the names match the pattern.

The documentation also says that the types condition should also come first in exports.

Can you correct those two things, and/or let me know if I'm incorrect about either of them?

@andipaetzold
Copy link
Contributor Author

I read through the Typescript documentation for this change (https://devblogs.microsoft.com/typescript/announcing-typescript-4-7/#package-json-exports-imports-and-self-referencing) and if I'm understanding it correctly, the types field is only needed when the typings file doesn't have the same name (and isn't in the same directory) as the entry point file. For example, if the entry point is dist/index.js then TypeScript will automatically expect the typings file to be dist/index.d.ts.

So the only files that should be broken are ones with non-matching names, mostly files with public in them, which are named that because they have gone through an additional step to remove non-public types. I don't think we need a types field in cases where the names match the pattern.

I wanted to be on the safe side here and be very explicit. I think this might prevent issues in case of refactorings in the future. E.g. if the types are moved to a different location and not referenced in the package.json, people might forget to add the types field. But, when a types path already exists, it might be more obvious that things need to be changed in the package.json.

In which package could the types be omitted? E.g. if the default is index.esm2017.js, the file index.d.ts is not automatically used but TypeScript searches for index.esm2017.d.ts.

Also, if there are multiple conditions (default, require, node), TypeScript will look for a different type declaration depending on which JavaScript file is used. So, the path must be explicitly defined there or we need to create more type declarations.

The documentation also says that the types condition should also come first in exports.

Will move.

I only read that default must be last. 🤔 If there is a new file that needs to be referenced, we are running out of positions in the object 😆

How do we think we could test whether all paths are correct? I twice through all paths, but I still might have referenced a missing type declaration. For a project this big, it might even make sense going through all export conditions (not only types) and verify that the file exists in the published bundle.

@hsubox76
Copy link
Contributor

hsubox76 commented Jun 1, 2022

Hmm, let me see if I can put together a script to check for that. I can run it locally on your branch to verify we can merge this PR, and then I will clean it up and submit it as a CI check later.

Copy link
Contributor

@hsubox76 hsubox76 left a comment

Choose a reason for hiding this comment

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

I ran a check and all the paths in this change resolve to at least something, except one, which is only incorrect because you copied it from the original, which was incorrect to begin with. I also found a few other incorrect paths in some parts you didn't touch, which I will fix when I create the PR for the path checking script.

I think this PR is good to go if you just fix the installations-compat path. Thanks again!

packages/installations-compat/package.json Outdated Show resolved Hide resolved
packages/installations-compat/package.json Outdated Show resolved Hide resolved
@hsubox76 hsubox76 merged commit 2cd1cc7 into firebase:master Jun 2, 2022
@google-oss-bot google-oss-bot mentioned this pull request Jun 9, 2022
@firebase firebase locked and limited conversation to collaborators Jul 3, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing type paths for TypeScript 4.7 Node16 modules
2 participants