alphagov / govuk-frontend

GOV.UK Frontend contains the code you need to start building a user interface for government platforms and services.
https://frontend.design-system.service.gov.uk/
MIT License
1.17k stars 320 forks source link

Consider JSDoc `@import` for non-exported types #5239

Open colinrotherham opened 1 month ago

colinrotherham commented 1 month ago

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 https://github.com/alphagov/govuk-frontend/issues/5157 by using @import instead

romaricpascal commented 1 month ago

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 commented 1 month ago

@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

romaricpascal commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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.

github-actions[bot] commented 1 month ago

Uh oh! @romaricpascal, the image you shared is missing helpful alt text. Check https://github.com/alphagov/govuk-frontend/pull/5239#issuecomment-2307459822.

Alt text is an invisible description that helps screen readers describe images to blind or low-vision users. If you are using markdown to display images, add your alt text inside the brackets of the markdown image.

Learn more about alt text at Basic writing and formatting syntax: images on GitHub Docs.

🤖 Beep boop! This comment was added automatically by github/accessibility-alt-text-bot.

github-actions[bot] commented 1 month ago

Uh oh! @romaricpascal, the image you shared is missing helpful alt text. Check https://github.com/alphagov/govuk-frontend/pull/5239#issuecomment-2307459822.

Alt text is an invisible description that helps screen readers describe images to blind or low-vision users. If you are using markdown to display images, add your alt text inside the brackets of the markdown image.

Learn more about alt text at Basic writing and formatting syntax: images on GitHub Docs.

🤖 Beep boop! This comment was added automatically by github/accessibility-alt-text-bot.

colinrotherham commented 1 month ago

Think you're all set

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

Thanks @romaricpascal