Skip to content

fix: limit logging to authoritative plugin version #5579

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

Merged
merged 4 commits into from
Apr 4, 2024
Merged

fix: limit logging to authoritative plugin version #5579

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fb52e60
chore: add system logging
eduardoboucas Apr 3, 2024
6948bc9
fix: limit logs to authoritative plugin version
eduardoboucas Apr 4, 2024
ca208e1
Merge branch 'main' into chore/system-log
eduardoboucas Apr 4, 2024
83dc1b2
fix: add missing property
eduardoboucas Apr 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions packages/build/src/plugins/compatibility.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,6 +282,7 @@ describe(`getExpectedVersion`, () => {
systemLog: (message: string) => {
logMessages.push(message)
},
authoritative: true,
})

expect(logMessages.length).toBe(1)
Expand Down Expand Up @@ -322,6 +323,7 @@ describe(`getExpectedVersion`, () => {
featureFlags: {
netlify_build_updated_plugin_compatibility: true,
},
authoritative: true,
})

expect(logMessages.length).toBe(1)
Expand Down
20 changes: 17 additions & 3 deletions packages/build/src/plugins/compatibility.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

build/packages/build/src/plugins/expected_version.ts

Lines 87 to 107 in 9b8ed35

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 -

build/packages/build/src/plugins/compatibility.ts

Lines 15 to 25 in 9b8ed35

/**
* 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

}: {
versions: PluginVersion[]
/** The package.json of the repository */
Expand All @@ -44,6 +45,11 @@ export const getExpectedVersion = async function ({
pinnedVersion?: string
featureFlags?: FeatureFlags
systemLog: SystemLogger
/* Defines whether the version returned from this method is the authoritative
version that will be used for the plugin; if not, the method may be called
just to get information about other compatible versions that will not be
selected */
authoritative?: boolean
}) {
const { version, conditions = [] } = await getCompatibleEntry({
versions,
Expand All @@ -54,7 +60,7 @@ export const getExpectedVersion = async function ({
buildDir,
pinnedVersion,
featureFlags,
systemLog,
systemLog: authoritative ? systemLog : undefined,
})

// Retrieve warning message shown when using an older version with `compatibility`
Expand Down Expand Up @@ -87,7 +93,9 @@ const getCompatibleEntry = async function ({
buildDir,
pinnedVersion,
featureFlags,
systemLog,
systemLog = () => {
// no-op
},
}: {
versions: PluginVersion[]
packageJson: PackageJson
Expand All @@ -97,7 +105,7 @@ const getCompatibleEntry = async function ({
packagePath?: string
pinnedVersion?: string
featureFlags?: FeatureFlags
systemLog: SystemLogger
systemLog?: SystemLogger
}): Promise<Pick<PluginVersion, 'conditions' | 'version'>> {
const compatibleEntry = await pLocate(versions, async ({ version, overridePinnedVersion, conditions }) => {
// When there's a `pinnedVersion`, we typically pick the first version that
Expand Down Expand Up @@ -126,10 +134,16 @@ const getCompatibleEntry = async function ({
})

if (compatibleEntry) {
systemLog(
`Used compatible version '${compatibleEntry.version}' for plugin '${packageName}' (pinned version is ${pinnedVersion})`,
)

return compatibleEntry
}

if (pinnedVersion) {
systemLog(`Used pinned version '${pinnedVersion}' for plugin '${packageName}'`)

return { version: pinnedVersion, conditions: [] }
}

Expand Down
1 change: 1 addition & 0 deletions packages/build/src/plugins/expected_version.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@ const addExpectedVersion = async function ({
pinnedVersion,
featureFlags,
systemLog,
authoritative: true,
}),
getExpectedVersion({
versions,
Expand Down