Resolve pkg: URLs from dist/govuk and update the review app#6839
Resolve pkg: URLs from dist/govuk and update the review app#6839romaricpascal wants to merge 3 commits intomainfrom
pkg: URLs from dist/govuk and update the review app#6839Conversation
📋 StatsNo changes to any distributed file sizes! Action run for 661aeb3 |
| "./package.json": "./package.json", | ||
| "./*": { | ||
| "sass": "./dist/govuk/*", | ||
| "default": "./*" |
There was a problem hiding this comment.
note Judging by the content of GOV.UK Frontend's package, I think we could also update this to ./dist/govuk/* to help people import JavaScript more easily.
I believe the change would not be breaking as README.md is. the only other file in the package outside of dist, govuk-prototype-kit.config.json, and package.json. We could just add an entry for it in the list of exports if we're worried about this one.
All in all, it's a separate matter than Sass module resolution, so best tackled in its own PR as we'd want some tests specific to JavaScript around it.
There was a problem hiding this comment.
Agree, this sounds like something to do in a follow up PR 👍🏻 It might also need some docs changes – think all of our JS examples at the minute import via all.mjs.
6f12b7a to
fb364a6
Compare
fb364a6 to
e821358
Compare
e821358 to
baf6631
Compare
tsconfig.base.json
Outdated
| "@govuk-frontend/tasks": ["./shared/tasks/index.mjs"], | ||
| "govuk-frontend": ["./packages/govuk-frontend/src/govuk/all.mjs"] | ||
| "govuk-frontend": ["./packages/govuk-frontend/src/govuk/all.mjs"], | ||
| "govuk-frontend/dist/govuk/components/button/button.mjs": [ |
There was a problem hiding this comment.
note This one probably needs some explaining.
The addition of a file importing the component file individually ends up making TypeScript process govuk-frontend/dist/govuk/components/button/button.mjs and the files it imports.
However... we do not ship types in GOV.UK Frontend, so TypeScript throws a bunch of errors when processing compiled files from GOV.UK Frontend.
Instead, we need to do like for govuk-frontend right above and point TypeScript to src.
There was a problem hiding this comment.
Suggestion: Would it make sense to add a tsconfig.json to tests or even tests/bundler-integrations that extends the base config and handles this there, so it's closer to the code that needs it?
Also, it looks like tsconfig.json allows comments 🤯 (tsc --init even adds some to the file) – so we could use that to document this inline.
There was a problem hiding this comment.
I could try for govuk-frontend/dist/govuk/components/button/button.mjs, however govuk-frontend needs to be a little more central as it's also used in the review app. Probably best to add a comment in there, that's a good shout.
tsconfig.base.json
Outdated
| "@govuk-frontend/tasks": ["./shared/tasks/index.mjs"], | ||
| "govuk-frontend": ["./packages/govuk-frontend/src/govuk/all.mjs"] | ||
| "govuk-frontend": ["./packages/govuk-frontend/src/govuk/all.mjs"], | ||
| "govuk-frontend/dist/govuk/components/button/button.mjs": [ |
There was a problem hiding this comment.
Suggestion: Would it make sense to add a tsconfig.json to tests or even tests/bundler-integrations that extends the base config and handles this there, so it's closer to the code that needs it?
Also, it looks like tsconfig.json allows comments 🤯 (tsc --init even adds some to the file) – so we could use that to document this inline.
| "./package.json": "./package.json", | ||
| "./*": { | ||
| "sass": "./dist/govuk/*", | ||
| "default": "./*" |
There was a problem hiding this comment.
Agree, this sounds like something to do in a follow up PR 👍🏻 It might also need some docs changes – think all of our JS examples at the minute import via all.mjs.
| join(basePath, 'node_modules'), | ||
| join(paths.root, 'node_modules') | ||
| ], | ||
| importers: [new NodePackageImporter()], |
|
|
||
| const { css } = await compileStringAsync(sass, sassConfig) | ||
|
|
||
| expect(css).toBe(cssWithPkg) |
There was a problem hiding this comment.
Nit: This feels backwards to me – I'd expect us to check that the Sass compiled using pkg: matches the Sass compiled using the more definite file path.
Appreciate it's functionally identical, but if they were different the diff would be the inverse of what the description for the describe block implies, I think?
There was a problem hiding this comment.
Similarly I'd expect the thing compiled outside of the test to be the fileUrl as the 'control' for the test.
There was a problem hiding this comment.
Sounds fair, I'll tidy the order on Monday 😊
baf6631 to
1483246
Compare
When using Sass, resolve any string after `pkg:govuk-frontend` from `dist/govuk` rather than the root of the package. This allows to reduce the length of `pkg:` URLs. The `./*` entry of `exports` has been moved to the bottom of the list as Node recommends ordering from more to least specific: > Within the "exports" object, key order is significant. During condition matching, earlier entries have higher priority and take precedence over later entries. The general rule is that conditions should be from most specific to least specific in object order. > - <https://nodejs.org/api/packages.html#self-referencing-a-package-using-its-name> This allows users to still access files explicitely using `dist/govuk` should they wish to, making this a non-breaking change if they were doing: `@use "pkg:govuk-frontend/dist/govuk"`. (In practice, it was also working with `./*` at the top of the list, I _think_ because no file matched `dist/govuk/dist/govuk` in the package so Node moved on to the next entry. But better safe than sorry.)
1483246 to
2f3bcfc
Compare
Ensures importing individual component files still resolves properly. Because GOV.UK Frontend does not provide type definitions in the built files, we need to do a similar mapping as for `govuk-frontend` and point to `src` rather than `dist`.
Upgrade styles compilation to use Sass' `NodePackageImporter` rather than a custom `loadPath`. This means we can now use `pkg:govuk-frontend` in the review app, which has been updated.
2f3bcfc to
661aeb3
Compare
When using Sass, resolve any string after
pkg:govuk-frontendfromdist/govukrather than the root of the package. This allows to reduce the length ofpkg:URLs:As a way to confirm things are working OK the PR also:
pkg:URLsBest reviewed commit by commit.
Fixes #6822