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

Only allow plugin update functionality when installed from npm #2356

Merged
merged 2 commits into from
Oct 11, 2023

Conversation

BenSurgisonGDS
Copy link
Contributor

@BenSurgisonGDS BenSurgisonGDS commented Oct 6, 2023

The kit had wrongly indicated that a plugin installed directly from github could be updated. This caused a user's prototype to crash when they attempted this. As the kit was only designed to allow updates from the npm registry, this change prevents the update button to appear for those plugins installed locally or from github. If the user wants to update their plugin installed from github, they'll have to upgrade it manually from the command line as they had installed it in the first place.

@BenSurgisonGDS BenSurgisonGDS changed the title Only allow update functionality when possible Only allow plugin update functionality when installed from npm Oct 9, 2023
@BenSurgisonGDS BenSurgisonGDS force-pushed the only-allow-update-functionality-when-possible branch from fac2ce1 to fbc819a Compare October 9, 2023 08:48
@BenSurgisonGDS BenSurgisonGDS marked this pull request as ready for review October 9, 2023 08:49
@BenSurgisonGDS BenSurgisonGDS force-pushed the only-allow-update-functionality-when-possible branch from fbc819a to 487d7ff Compare October 9, 2023 08:50
@BenSurgisonGDS BenSurgisonGDS self-assigned this Oct 9, 2023
@BenSurgisonGDS BenSurgisonGDS force-pushed the only-allow-update-functionality-when-possible branch from 487d7ff to b062961 Compare October 9, 2023 10:45
@@ -87,6 +87,7 @@ async function refreshPackageInfo (packageName, version) {
const installedPackageVersion = packageJson && projectPackage.dependencies[packageName]
Copy link
Contributor

Choose a reason for hiding this comment

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

@BenSurgisonGDS Might be a good idea to use .packages[packageName]

The legacy dependencies[packageName] entry is dropped in newer npm versions and only exists for compatibility in older projects created before "lockfileVersion": 3 was the default

You can use the CLI to switch lockfile version:

npm install --lockfile-version 2
npm install --lockfile-version 3

Which will show the legacy dependencies no longer exists in v3:

 {
   "name": "test-prototype",
   "lockfileVersion": 3,
   "packages": {
     "../../../../../../../path/to/govuk-prototype-kit": {
       "version": "13.13.4",
       "dependencies": {}
     }
-  },
-  "dependencies": {
-    "govuk-prototype-kit": {
-      "version": "file:../../../../../../../Users/colin/Sites/GDS/govuk-prototype-kit",
-      "requires": {}
-    }
+  }
 }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it would be great to do this in a future ticket

@@ -87,6 +87,7 @@ async function refreshPackageInfo (packageName, version) {
const installedPackageVersion = packageJson && projectPackage.dependencies[packageName]
const installed = !!installedPackageVersion
const installedLocally = installedPackageVersion?.startsWith('file:')
const installedFromGithub = installedPackageVersion?.startsWith('github')
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably want to run resolved through new URL() confirming hostname as registry.npmjs.org?

{
  "name": "test-prototype",
  "lockfileVersion": 3,
  "packages": {
    "node_modules/govuk-prototype-kit": {
      "version": "13.13.4",
      "resolved": "https://registry.npmjs.org/govuk-prototype-kit/-/govuk-prototype-kit-13.13.4.tgz",
      "integrity": "sha512-P4HfYXIAUpjWe5NIfTEYxOkg0iE9GuQBmJ0KO+KoFVkFre2yPlAoa3kQA30UjjvtT2+c+Sc70Abkx424rwaiAg==",
      "dependencies": {}
    }
  }
}

Copy link
Contributor

Choose a reason for hiding this comment

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

For now though, shall we prefer github: (with a colon) consistent with file:?

Suggested change
const installedFromGithub = installedPackageVersion?.startsWith('github')
const installedFromGithub = installedPackageVersion?.startsWith('github:')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done as suggested

@@ -87,6 +87,7 @@ async function refreshPackageInfo (packageName, version) {
const installedPackageVersion = packageJson && projectPackage.dependencies[packageName]
const installed = !!installedPackageVersion
const installedLocally = installedPackageVersion?.startsWith('file:')
Copy link
Contributor

Choose a reason for hiding this comment

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

For another PR, but this will be "link": true in the new lockfile format too

@colinrotherham colinrotherham self-requested a review October 9, 2023 15:17
colinrotherham
colinrotherham previously approved these changes Oct 9, 2023
Copy link
Contributor

@colinrotherham colinrotherham left a comment

Choose a reason for hiding this comment

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

Does what it says ✅

Related but not blocking, could do with writing up an issue for:

  • Compatibility with npm v9 "lockfileVersion": 3
  • Suppressing link "New version available" for linked kit package (see below)
Update link for the kit package

nataliecarey
nataliecarey previously approved these changes Oct 10, 2023
@BenSurgisonGDS BenSurgisonGDS force-pushed the only-allow-update-functionality-when-possible branch from 74b94e9 to 4fe2805 Compare October 11, 2023 09:38
@BenSurgisonGDS BenSurgisonGDS merged commit e2c63d1 into main Oct 11, 2023
27 checks passed
@BenSurgisonGDS BenSurgisonGDS deleted the only-allow-update-functionality-when-possible branch October 11, 2023 10:24
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.

3 participants