Skip to content

[ui] Namespaced and file-specified deps for consul-ui#21378

Merged
philrenaud merged 5 commits into
mainfrom
NET-10157-consul-ui-devdep-specificity
Jul 8, 2024
Merged

[ui] Namespaced and file-specified deps for consul-ui#21378
philrenaud merged 5 commits into
mainfrom
NET-10157-consul-ui-devdep-specificity

Conversation

@philrenaud
Copy link
Copy Markdown
Collaborator

Description

This resolves an ambiguous dependency resolution in consul-ui in the event that a developer attempted to yarn install or npm install from a non-workspace-root directory. This handles it in two ways:

  • Namespaces with a @hashicorp/ prefix, reducing the risk of a hypothetical public registry squat situation should local packages ever stop being relative
  • Explicitly locally dependency-links packages

Something to note with this is that the packages become a little more bound together than before; for example, it would be more difficult to maintain multiple valid versions of a sub-package here and let consul-ui differentiate between them. We could expand this into submodules some day, but not today.

@philrenaud philrenaud self-assigned this Jun 27, 2024
@github-actions github-actions Bot added the theme/ui Anything related to the UI label Jun 27, 2024
@dduzgun-security
Copy link
Copy Markdown
Contributor

Awesome work @philrenaud, thanks a lot for doing that this quickly 🙌
One small comment about consul-ui, should we bump the version to 2.2.1?

@dduzgun-security dduzgun-security added the pr/needs-rebase PR needs to be rebased before merging label Jun 27, 2024
Comment thread ui/packages/consul-ui/package.json
@philrenaud philrenaud force-pushed the NET-10157-consul-ui-devdep-specificity branch from 4007e05 to e18b631 Compare July 2, 2024 19:48
@philrenaud
Copy link
Copy Markdown
Collaborator Author

Note: in order to resolve a non-standard license in a dependency-of-dependency-of-dependency, in 36fef17e6cdc9dbb088de1bbad9bc4ed44eee6ee I've done two things:

  • pinned tailwindcss to a specific version that no longer imports the dependency chain that would ultimately include a non-standard license
  • made a config file at .yarnrc to specify that package resolution should come from registry.npmjs.org rather than registry.yarnpkg.com. This was a precedent set in Upgrade Consul UI to Node 18 (#19252) #19362 (see yarn lock for all the domain changes) likely due to an environment variable for the upgrader at the time. This change enshrines it.

@philrenaud philrenaud force-pushed the NET-10157-consul-ui-devdep-specificity branch from ee4cc11 to 7277670 Compare July 6, 2024 14:07
@philrenaud philrenaud added backport/1.19 backport/all Apply backports for all active releases per .release/versions.hcl and removed pr/needs-rebase PR needs to be rebased before merging backport/1.19 labels Jul 6, 2024
@philrenaud philrenaud force-pushed the NET-10157-consul-ui-devdep-specificity branch 2 times, most recently from a2e4715 to 79d8b85 Compare July 8, 2024 19:53
@philrenaud philrenaud force-pushed the NET-10157-consul-ui-devdep-specificity branch from 79d8b85 to 8d13103 Compare July 8, 2024 20:15
Copy link
Copy Markdown
Contributor

@dduzgun-security dduzgun-security left a comment

Choose a reason for hiding this comment

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

LGTM! Huge thanks for fixing this and for all the details you have provided for the resolution, it's awesome 👍
We can merge once tests are passing

@philrenaud philrenaud merged commit dce6241 into main Jul 8, 2024
@philrenaud philrenaud deleted the NET-10157-consul-ui-devdep-specificity branch July 8, 2024 20:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport/all Apply backports for all active releases per .release/versions.hcl backport/ent/1.18 Changes are backported to 1.18 ent theme/ui Anything related to the UI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants