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.

Sign up for GitHub

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

Jump to bottom

Specify plugin dependencies #2220

Merged
merged 4 commits into from
Jul 4, 2023
Merged

Specify plugin dependencies #2220

merged 4 commits into from
Jul 4, 2023

Conversation

BenSurgisonGDS
Copy link
Contributor

@BenSurgisonGDS BenSurgisonGDS commented Jun 8, 2023 •
edited
Loading

See: Allow plugin developers to specify that their plugin is dependent on other plugins

  • Improve efficiency of loading and combining plugin info both locally and from the npm repository
  • Implement the "pluginDependencies" property within the plugin config to specify required plugins
  • Allow users to install and uninstall local plugins to aid in plugin development
  • Determine whether installing a plugin requires the installation of additional plugins if they are not already installed
  • Determine whether uninstalling a plugin requires the uninstall of depenent plugins if they are currently installed
  • Implement specified page layouts for installing and uninstalling when dependencies affect the process
  • Update the plugins validator to accept the new "pluginDependencies" property
  • Add or amend unit and integration tests for all the above changes
  • Add Cypress tests to prove the above functionality

@BenSurgisonGDS BenSurgisonGDS linked an issue Jun 8, 2023 that may be closed by this pull request
Allow plugin developers to specify that their plugin is dependent on other plugins #2125
Closed
3 tasks
@BenSurgisonGDS BenSurgisonGDS self-assigned this Jun 8, 2023
@BenSurgisonGDS BenSurgisonGDS force-pushed the 2125-allow-plugin-developers-to-specify-that-their-plugin-is-dependent-on-other-plugins branch 12 times, most recently from d979b06 to f712d80 Compare June 15, 2023 11:42
@BenSurgisonGDS BenSurgisonGDS force-pushed the 2125-allow-plugin-developers-to-specify-that-their-plugin-is-dependent-on-other-plugins branch 5 times, most recently from 1a1d3ed to 8236d62 Compare June 21, 2023 15:59
@BenSurgisonGDS BenSurgisonGDS marked this pull request as ready for review June 21, 2023 16:17
@BenSurgisonGDS BenSurgisonGDS force-pushed the 2125-allow-plugin-developers-to-specify-that-their-plugin-is-dependent-on-other-plugins branch 4 times, most recently from d1ee672 to 5971a44 Compare June 26, 2023 18:57
nataliecarey
nataliecarey reviewed Jun 27, 2023
View reviewed changes
proof-of-concept.js Outdated
@@ -0,0 +1,19 @@
const { startPerformanceTimer, endPerformanceTimer } = require('./lib/utils/performance')
Copy link
Contributor

Choose a reason for hiding this comment

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

We can remove the Proof of Concept now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed as requested

nataliecarey
nataliecarey reviewed Jun 27, 2023
View reviewed changes
known-plugins.json Outdated
@@ -9,6 +9,10 @@
"hmrc-frontend",
"jquery",
"notifications-node-client"
],
"mandatory": [
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm nervous about "mandatory" including govuk-frontend, it might be a naming thing. I presume this is to separate out the ones that the user can't uninstall in the UI at the moment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's correct

Copy link
Contributor

Choose a reason for hiding this comment

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

@joelanman I think the reason we removed the uninstall button was because this feature was needed before we could have it in. Should we put the uninstall button back in alongside govuk-frontend now that dependent plugins dealt with?

Copy link
Contributor

Choose a reason for hiding this comment

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

That was one reason, but I think it was more general than that - that the user experience in general relies on Frontend - if it's missing things look and feel broken. It'd be good to review the user experience when Frontend is removed to see what it's like. Seems like that work would block this, so maybe lets leave 'mandatory' for now and remove when we're ready

Copy link
Contributor

Choose a reason for hiding this comment

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

just thinking 'required' might be better than 'mandatory' - one to run by @oli-rose28

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 agree that "required" is a better name than "mandatory" so I have made the change as suggested.

nataliecarey
nataliecarey reviewed Jun 27, 2023
View reviewed changes
lib/plugins/plugin-validator.js Outdated
if (typeof configEntry.packageName !== 'string') {
errors.push(`In section ${key}, the packageName '${configEntry.packageName}' should be a valid package name`)
}
if (typeof configEntry.minVersion !== 'string') {
Copy link
Contributor

Choose a reason for hiding this comment

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

This code suggests that minVersion has to be set, I was expecting both min and max to be optional and to accept strings as well as objects. e.g. I would see each of these as valid configs

"pluginDependencies": ["hmrc-frontend"]

"pluginDependencies": [{
  "packageName": "@x-govuk/edit-prototype-in-browser",
  "maxVersion": "1.0.0"
}]

"pluginDependencies": [{
  "packageName": "@x-govuk/edit-prototype-in-browser",
  "minVersion": "1.0.0"
}]

"pluginDependencies": [{
  "packageName": "@x-govuk/edit-prototype-in-browser",
  "minVersion": "0.1.1",
  "maxVersion": "1.0.0"
}]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed as requested including some extra unit tests.

nataliecarey
nataliecarey reviewed Jun 27, 2023
View reviewed changes
lib/plugins/packages.js Outdated
])

if ([packageJson, pluginConfig, registryInfo].every(val => val === undefined)) {
if (!version) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason why version isn't in the array with the others?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I've added it

@BenSurgisonGDS BenSurgisonGDS force-pushed the 2125-allow-plugin-developers-to-specify-that-their-plugin-is-dependent-on-other-plugins branch 3 times, most recently from 1f27f8a to 81f4e71 Compare June 30, 2023 11:44
@nataliecarey nataliecarey force-pushed the 2125-allow-plugin-developers-to-specify-that-their-plugin-is-dependent-on-other-plugins branch 2 times, most recently from 0976800 to b5007ba Compare July 4, 2023 09:02
nataliecarey
nataliecarey reviewed Jul 4, 2023
View reviewed changes
lib/plugins/packages.js Outdated
const packagesCache = {}

// This allows npm modules to act as if they are plugins by providing the plugin config for them
const proxyPluginConfig = {
Copy link
Contributor

Choose a reason for hiding this comment

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

This has been duplicated, we should keep one version (if it's needed in both places we should look up that one version in both places)

Copy link
Contributor Author

@BenSurgisonGDS BenSurgisonGDS Jul 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I'll remove the duplicate

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the duplication and prevented a circular dependency

@nataliecarey
Copy link
Contributor

I think it's worth squashing the commits. Then it looks ready.

@BenSurgisonGDS BenSurgisonGDS force-pushed the 2125-allow-plugin-developers-to-specify-that-their-plugin-is-dependent-on-other-plugins branch from e1b09b8 to 5f904cd Compare July 4, 2023 14:27
@BenSurgisonGDS BenSurgisonGDS force-pushed the 2125-allow-plugin-developers-to-specify-that-their-plugin-is-dependent-on-other-plugins branch from 5f904cd to 015d0ea Compare July 4, 2023 14:28
@BenSurgisonGDS BenSurgisonGDS merged commit f1dcd55 into main Jul 4, 2023
@BenSurgisonGDS BenSurgisonGDS deleted the 2125-allow-plugin-developers-to-specify-that-their-plugin-is-dependent-on-other-plugins branch July 4, 2023 14:45
@BenSurgisonGDS BenSurgisonGDS mentioned this pull request Jul 5, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joelanman joelanman joelanman left review comments

@nataliecarey nataliecarey nataliecarey approved these changes

@HannahJMWood HannahJMWood Awaiting requested review from HannahJMWood

Assignees

@BenSurgisonGDS BenSurgisonGDS

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow plugin developers to specify that their plugin is dependent on other plugins
3 participants
@BenSurgisonGDS @nataliecarey @joelanman