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

Consider JSDoc @import for non-exported types #5239

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

colinrotherham
Copy link
Contributor

Hello 👋

Thought I'd share a little improvement added in TypeScript 5.5: JSDoc @import tag

Should prevent @typedef re-exporting private/internal definitions in #5157 by using @import instead

@romaricpascal
Copy link
Member

Oh, that's nice, thanks for pointing that out and updating the existing @typedef 🙌🏻 . Is the PR in draft because you're expecting to make further changes?

@colinrotherham
Copy link
Contributor Author

@romaricpascal No problem 😊

Nah only in "draft" because I've only rolled out @import within source code, it's good to go if you're happy. I was being nosey and hoped it might ease up the missing exports in 75e6e14

I'd recommend widely using @import as it preserves all the generics too, so no need for @template

Just pushed an extra commit to demonstrate

@colinrotherham colinrotherham marked this pull request as ready for review August 19, 2024 16:21
@romaricpascal
Copy link
Member

Thanks for marking as ready for review. I'll approve the workflows and have a look Thursday or Friday, hopefully it'll play nicely with the TypeDoc config update (whose aim is to have the types we don't export in the doc so you'd know what shape an AccordionConfig is, for example).

Happy to take over updating the rest of the import() in types, as well. Would it be fine if they were pushed on your branch so we merged them all in one go?

I'd recommend widely using @import as it preserves all the generics too, so no need for @template

Just to confirm, is it this part of the commit that statement refers to, where you were able to drop the @typedef for HandlerFunction and the @template it required to keep it being a generics because we were now able to directly use EvaluateFuncWith via @import?

@colinrotherham
Copy link
Contributor Author

Happy to take over updating the rest of the import() in types, as well. Would it be fine if they were pushed on your branch so we merged them all in one go?

Yeah go for it ✅

Just to confirm, is it this part of the commit that statement refers to, where you were able to drop the @typedef for HandlerFunction and the @template it required to keep it being a generics because we were now able to directly use EvaluateFuncWith via @import?

That's the one

Previously all generics needed to be re-declared via @template, now they just work

@romaricpascal
Copy link
Member

romaricpascal commented Aug 23, 2024

Yeah go for it ✅

Cheers! I've rebased on main and the @import tag seems to play nicely with the updates we recently made to TypeDoc for listing more internal types in the review app.

I've also added a round of updates for the @typedef {import(...)} tags in other parts of our code. The @import syntax is pretty neat when importing multiple types from a single file (and showed a couple of places we were importing unnecessarily). I didnd't see any obvious @template ones to update but we can always fix those later if there are any.

Looking into this further, I'm thinking one of the effect of using @import for the sources will be that the info within JSDoc blocks will only compatible with TypeScript 5.5 and further (as older versions won't understand the tag). This means only people running TypeScript 5.5 will get the knowledge that Config['accordion'] is an AccordionConfig resolving to the {i18n: AccordionTranslations, rememberExpanded: boolean} type.

For earlier versions, AccordionConfig will resolve to any if I followed correctly what was happening before the @import tags being kept in the built output.

With `@import` support Without `@import` support
Completion Completions show `i18n` and `rememberExpanded` when calling `initAll`. Completions do not show `i18n` and `rememberExpanded` when calling `initAll`.
Resolution of `AccordionConfig` Resolved type for `AccordionConfig` in built `init.mjs` shows all expected properties (`i18n` and `rememberExpanded`). Resolved type for `AccordionConfig` in built  `init.mjs` is `any`.

This shouldn't break existing builds, just loosen the type checking a little (again, if I understand correctly). Will have a check with the team, but it doesn't feel like a blocker to start using @import.

This comment was marked as resolved.

1 similar comment

This comment was marked as resolved.

@colinrotherham
Copy link
Contributor Author

colinrotherham commented Aug 23, 2024

Think you're all set

IDEs and editors will discover @types/govuk-frontend now and this repo uses TypeScript 5.5+ already

Thanks @romaricpascal

colinrotherham and others added 7 commits October 11, 2024 15:37
The one in `tasks/index.mjs` is left as it renames the import and there does not seem to be a syntax for that with the `@import` tag
Without it, internal types like `AccordionConfig` will be unresolved in the built files as the `@import` comments are dropped
Copy link
Member

@romaricpascal romaricpascal left a comment

Choose a reason for hiding this comment

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

I've rebased the PR on main and it looks good to me. It'll need a second approval before we can merge it though.

@romaricpascal romaricpascal requested a review from a team October 11, 2024 14:52
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