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

fix: skip the plugin if it has been called before with the same id and importer #19016

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
57 changes: 57 additions & 0 deletions packages/vite/src/node/server/__tests__/pluginContainer.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -258,6 +258,63 @@ describe('plugin container', () => {
const result = await environment.pluginContainer.resolveId('foo')
expect(result).toStrictEqual({ id: 'success' })
})

it('should skip the plugin if it has been called before with the same id and importer (1)', async () => {
const p1: Plugin = {
name: 'p1',
async resolveId(id, importer) {
return (
(await this.resolve(id.replace(/\/modified$/, ''), importer, {
skipSelf: true,
})) ?? 'success'
)
},
}
const p2: Plugin = {
name: 'p2',
async resolveId(id, importer) {
return await this.resolve(id + '/modified', importer, {
skipSelf: true,
})
},
}
const environment = await getDevEnvironment({ plugins: [p1, p2] })
const result = await environment.pluginContainer.resolveId('foo')
expect(result).toStrictEqual({ id: 'success' })
})

it('should skip the plugin if it has been called before with the same id and importer (2)', async () => {
const p1: Plugin = {
name: 'p1',
async resolveId(id, importer) {
return (
(await this.resolve(id.replace(/\/modified$/, ''), importer, {
skipSelf: true,
})) ?? 'failure1'
)
},
}
const p2: Plugin = {
name: 'p2',
async resolveId(id, importer) {
return await this.resolve(id + '/modified', importer, {
skipSelf: true,
})
},
}
const p3: Plugin = {
name: 'p3',
resolveId(id) {
if (id.endsWith('/modified')) {
return 'success'
}
return 'failure2'
},
}
const environment = await getDevEnvironment({ plugins: [p1, p2, p3] })
const result = await environment.pluginContainer.resolveId('foo')
expect(result).toStrictEqual({ id: 'success' })
})
})
})
})
Expand Down
32 changes: 24 additions & 8 deletions packages/vite/src/node/server/pluginContainer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,7 @@ export type SkipInformation = {
id: string
importer: string | undefined
plugin: Plugin
called?: boolean
}

class EnvironmentPluginContainer {
Expand Down Expand Up @@ -364,7 +365,7 @@ class EnvironmentPluginContainer {

const mergedSkip = new Set<Plugin>(skip)
for (const call of skipCalls ?? []) {
if (call.id === rawId && call.importer === importer) {
if (call.called || (call.id === rawId && call.importer === importer)) {
mergedSkip.add(call.plugin)
}
}
Expand Down Expand Up @@ -576,13 +577,28 @@ class PluginContext implements Omit<RollupPluginContext, 'cache'> {
skipSelf?: boolean
},
) {
const skipCalls =
options?.skipSelf === false
? this._resolveSkipCalls
: [
...(this._resolveSkipCalls || []),
{ id, importer, plugin: this._plugin },
]
let skipCalls: readonly SkipInformation[] | undefined
if (options?.skipSelf === false) {
skipCalls = this._resolveSkipCalls
} else if (this._resolveSkipCalls) {
const skipCallsTemp = [...this._resolveSkipCalls]
const sameCallIndex = this._resolveSkipCalls.findIndex(
(c) =>
c.id === id && c.importer === importer && c.plugin === this._plugin,
)
if (sameCallIndex !== -1) {
skipCallsTemp[sameCallIndex] = {
...skipCallsTemp[sameCallIndex],
called: true,
}
Comment on lines +589 to +593
Copy link
Collaborator

@hi-ogawa hi-ogawa Dec 20, 2024

Choose a reason for hiding this comment

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

Awesome investigation!
The fix looks good to merge, but while trying to understand the logic, I cannot tell why we cannot immediately do return null for such call as this seems to be destined to go resolve loop.
The new test case seems to pass with "just return null". Do you have a case such approach fails?

Copy link
Member Author

Choose a reason for hiding this comment

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

I added a test case that fails with return null: 67c4a66

} else {
skipCallsTemp.push({ id, importer, plugin: this._plugin })
}
skipCalls = skipCallsTemp
} else {
skipCalls = [{ id, importer, plugin: this._plugin }]
}

let out = await this._container.resolveId(id, importer, {
attributes: options?.attributes,
custom: options?.custom,
Expand Down
Loading