Skip to content

Conversation

eduardoboucas
Copy link
Member

Summary

Adds additional system logging to the plugin version resolution logic.

@eduardoboucas eduardoboucas requested review from a team as code owners April 3, 2024 18:20
@eduardoboucas eduardoboucas changed the title chore: add system logging to plugin resolution fix: limit logging to authoritative plugin version Apr 4, 2024
@@ -33,6 +33,7 @@ export const getExpectedVersion = async function ({
pinnedVersion,
featureFlags,
systemLog,
authoritative,
Copy link
Contributor

Choose a reason for hiding this comment

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

getExpectedVersion({
versions,
nodeVersion,
packageJson,
packageName,
packagePath,
buildDir,
pinnedVersion,
featureFlags,
systemLog,
}),
getExpectedVersion({
versions,
nodeVersion,
packageJson,
packageName,
packagePath,
buildDir,
featureFlags,
systemLog,
}),
is not setting it, so this would just mute existing log and all the ones added in this PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

Just not clear to me which one of those should be authoritative one :S

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops! My bad. I was passing it to the unit test and saw it working, but forgot to add it to the actual place it needs to be.

Fixed in 83dc1b2.

Copy link
Member Author

Choose a reason for hiding this comment

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

Just not clear to me which one of those should be authoritative one :S

I think it needs to be the one that sends the pinned version, which is the one that decides which version to use. Otherwise we would never take the pinned version into account.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah -

/**
* Retrieve the `expectedVersion` of a plugin:
* - This is the version which should be run
* - This takes version pinning into account
* - If this does not match the currently cached version, it is installed first
* This is also used to retrieve the `compatibleVersion` of a plugin
* - This is the most recent version compatible with this site
* - This is the same logic except it does not use version pinning
* - This is only used to print a warning message when the `compatibleVersion`
* is older than the currently used version.
*/
this also confirms it

@eduardoboucas eduardoboucas enabled auto-merge (squash) April 4, 2024 08:48
@eduardoboucas eduardoboucas merged commit 6263d87 into main Apr 4, 2024
@eduardoboucas eduardoboucas deleted the chore/system-log branch April 4, 2024 08:55
This was referenced Jun 26, 2024
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.

2 participants