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

feat: Add type exports to enable declaration: true in tsconfig.json #1509

Merged
merged 7 commits into from
Nov 8, 2024

Conversation

osaton
Copy link
Contributor

@osaton osaton commented Nov 4, 2024

Re: #1014

Fix the type issues encountered when re-exporting types in a client application with compilerOptions.declaration set to true. This change addresses errors such as:

The inferred type of 'Link' cannot be named without a reference to '../node_modules/next-intl/dist/types/src/shared/types'. This is likely not portable. A type annotation is necessary.ts(2742)

Copy link

vercel bot commented Nov 4, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
next-intl-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 8, 2024 9:53am
next-intl-example-app-router ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 8, 2024 9:53am
next-intl-example-app-router-without-i18n-routing ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 8, 2024 9:53am

Copy link

vercel bot commented Nov 4, 2024

@osaton is attempting to deploy a commit to the next-intl Team on Vercel.

A member of the Team first needs to authorize it.

Copy link
Owner

@amannn amannn left a comment

Choose a reason for hiding this comment

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

Super cool, thanks a lot for looking into this!

HrefOrHrefWithParams,
HrefOrUrlObjectWithParams,
QueryParams
} from '../shared/utils';
Copy link
Owner

Choose a reason for hiding this comment

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

Is there a way to get a list of which types need to be exported? Or was this rather trial & error?

Some of these types should have their names reconsidered in case we have to export them. Ideally, we wouldn't have to. In the past, I think I was able to work around exporting some types via ReturnType<typeof …> annotations at some places where I saw declaration errors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, it's just trial & error. At least I don't know of a better way to check them. That said, this approach led to some unnecessary exports which I’ve removed in my latest commit.

Copy link
Owner

Choose a reason for hiding this comment

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

Okay, thanks! I'll also verify the imports again once we can use example-app-router-playground to verify that declaration: true works.

- "examples/*"
- "docs"
- 'packages/*'
- 'type-export-test'
Copy link
Owner

Choose a reason for hiding this comment

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

example-app-router-playground is in fact already a good example that stress-tests all APIs of next-intl. Can we add a tsconfig.declaration.json there and add a lint task that calls tsc with that config?

That way we don't have to add a completely new app (each example adds some maintenance burden).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we add some directory for export tests, or how should we approach this? example-app-router-playground already tests most things, but it does not cover export of every return type from the library.

Copy link
Owner

Choose a reason for hiding this comment

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

Which APIs did you find are not used in example-app-router-playground yet? If there are some, you can add e.g. a component in the src/components folder that uses the API. It doesn't even need to be imported anywhere, as this is just about ensuring tsc runs properly. But please check if they aren't already used somewhere 🙂.

Copy link
Contributor Author

@osaton osaton Nov 7, 2024

Choose a reason for hiding this comment

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

They are used, but not everything is exported. For example useTimeZone hook is only used inside components and as they only export JSX.Element, TypeScript does not have to try and export the return type of that function. But if you add something like:

export function useExport() {
  return useTimeZone();
}

You'll get the following error:

The inferred type of 'useExport' cannot be named without a reference to '../../../../packages/next-intl/node_modules/use-intl/dist/types/src/core/TimeZone'. This is likely not portable. A type annotation is necessary.

Copy link
Owner

Choose a reason for hiding this comment

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

Ah, gotcha! Do you think you can add a module to example-app-router-playground that contains all such cases along with a lint task for the declaration? I'd really like to avoid adding a new example.

example-app-router-playground already tests a bunch of functionality, might be handy also for the declaration test to ensure all of the demonstrated patterns work with this mode.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The export tests are now located in example-app-router-playground. I updated the app's existing tsconfig.json with the declaration property, as it improves editor feedback.

I removed createSharedPathnamesNavigation and createLocalizedPathnamesNavigation from export tests since they are deprecated, and createNavigation covers both of those.

Feel free to make changes, if you'd like to tweak anything.

Copy link
Owner

@amannn amannn Nov 8, 2024

Choose a reason for hiding this comment

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

Awesome, thanks a lot! Just had another look from my side and added a commit with some minor cleanup.

All good from my perspective, let's see if CI is happy too …

 - Revert change in turbo.json
 - Add tests for async APIs and createNextIntlPlugin
 - Remove not needed type exports: StrictParams, InitializedLocaleCookieConfig
Copy link
Owner

@amannn amannn left a comment

Choose a reason for hiding this comment

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

Thanks a bunch for your help here @osaton! 👏

@amannn amannn merged commit 6b2ca9c into amannn:main Nov 8, 2024
7 checks passed
@osaton osaton deleted the type-export-fixes branch November 8, 2024 10:24
juanforlizzi pushed a commit to juanforlizzi/next-intl that referenced this pull request Jan 16, 2025
…n` (amannn#1509)

Re: <amannn#1014>

Fix the type issues encountered when re-exporting types in a client
application with `compilerOptions.declaration` set to `true`. This
change addresses errors such as:

> The inferred type of 'Link' cannot be named without a reference to
'../node_modules/next-intl/dist/types/src/shared/types'. This is likely
not portable. A type annotation is necessary.ts(2742)

---------

Co-authored-by: Jan Amann <jan@amann.work>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants