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

Update package helpers to support govuk-frontend@4 #3757

Merged
merged 6 commits into from
Jun 9, 2023

Conversation

colinrotherham
Copy link
Contributor

@colinrotherham colinrotherham commented Jun 6, 2023

This PR updates our require.resolve() package resolution to work with GOV.UK Frontend v4

As a workaround for #3755 packageTypeToPath() can resolve paths via the main or module fields instead

Rollup stats for old releases

Package stats can now be built for govuk-frontend@4 as shown on branch package-resolver-v4

npm version 5.0.0 --no-git-tag-version --workspace govuk-frontend
npm install govuk-frontend@4.6.0 --save --workspace govuk-frontend-stats

Note: It's necessary to bump our workspace govuk-frontend to v5.0.0 to avoid conflict

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3757 June 6, 2023 11:56 Inactive
@colinrotherham colinrotherham changed the title [WIP] Update our Node.js package helpers use govuk-frontend@4 Update Node.js package helpers to support govuk-frontend@4 Jun 6, 2023
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3757 June 6, 2023 12:08 Inactive
@colinrotherham colinrotherham changed the title Update Node.js package helpers to support govuk-frontend@4 Update package helpers to support govuk-frontend@4 Jun 6, 2023
@@ -81,8 +87,8 @@ export async function compileStylesheet ([modulePath, { configPath, srcPath, des

// Resolve @imports via
loadPaths: [
packageNameToPath('govuk-frontend', 'dist'),
join(paths.root, 'node_modules')
packageNameToPath('govuk-frontend', 'dist', { requirePaths }),
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 need it, this line ensures govuk-frontend is resolved locally from the review app first

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3757 June 6, 2023 13:53 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3757 June 6, 2023 14:11 Inactive
@colinrotherham colinrotherham changed the base branch from glob-syntax to main June 7, 2023 17:29
@colinrotherham colinrotherham changed the base branch from main to glob-syntax June 7, 2023 17:29
@colinrotherham colinrotherham force-pushed the package-resolver branch 2 times, most recently from 620a851 to 4071c94 Compare June 7, 2023 17:58
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3757 June 7, 2023 17:59 Inactive
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.

Nice to be able to pick our version of GOV.UK frontend and good path for resolving things like import.meta.resolve 🙌🏻 Just a couple of suggestions that I think would help us touch this area of the code more easily going forward (one for naming and one for the resolving)? No worries if they miss the mark completely though.

shared/lib/names.js Outdated Show resolved Hide resolved
packages/govuk-frontend-review/rollup.config.mjs Outdated Show resolved Hide resolved
shared/config/resolver.js Outdated Show resolved Hide resolved
Base automatically changed from glob-syntax to main June 9, 2023 10:55
Preparation work to simplify how we call our package helpers, options param coming soon
Used to resolve a package’s main entry point path by name, or by resolving a full entry path using `require.resolve()` against the package’s exports
Whilst Node.js `import.meta.resolve()` is flagged experimental, this helper enables `module` field access for ES modules
Allows us to switch between `govuk-frontend@5` and `govuk-frontend@5` without issue
Node.js `require.resolve()` paths are optionally provided, for example:

1. Review app uses local `node_modules` before project level
2. Package stats uses local `node_modules` before project level

This is necessary for workspace `govuk-frontend@4` installs to be found
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3757 June 9, 2023 16:14 Inactive
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.

Cheers for the changes, looks good to go for me! ⛵

Comment on lines +81 to +103
/**
* Return path to package entry from any npm workspace, by type
*
* Wraps {@link packageResolveToPath} to allow the appended `modulePath` to
* include unresolvable paths, globs or files that are not yet built
*
* {@link https://github.com/alphagov/govuk-frontend/issues/3755}
*
* @example
* Resolving components relative to a default package entry
*
* - GOV.UK Frontend v4 './govuk/components/accordion/accordion.mjs'
* - GOV.UK Frontend v5 './dist/govuk/components/accordion/accordion.mjs'
*
* ```mjs
* const templatePath = packageResolveToPath('govuk-frontend', {
* modulePath: `components/accordion/accordion.mjs`
* })
* ```
* @param {string} packageName - Installed npm package name
* @param {PackageOptions} [options] - Package resolution options
* @returns {string} Path to installed npm package field
*/
Copy link
Member

Choose a reason for hiding this comment

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

Super neat and nice solution for the naming as well 😍

@colinrotherham colinrotherham merged commit dc24b72 into main Jun 9, 2023
@colinrotherham colinrotherham deleted the package-resolver branch June 9, 2023 16:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

3 participants