Break infinite loops in this.resolve #4000
Conversation
Thank you for your contribution! ❤️You can try out this pull request locally by installing Rollup via or load it into the REPL: |
Codecov Report
@@ Coverage Diff @@
## master #4000 +/- ##
=======================================
Coverage 97.24% 97.24%
=======================================
Files 191 192 +1
Lines 6742 6755 +13
Branches 1968 1971 +3
=======================================
+ Hits 6556 6569 +13
Misses 99 99
Partials 87 87
Continue to review full report at Codecov.
|
lukastaegert
force-pushed
the
break-infinite-this-resolve
branch
from
ce27d5c to
55d1198
Compare
| { | ||
| name: 'second', | ||
| async resolveId(source, importer) { | ||
| const { id } = await this.resolve(source, importer, { skipSelf: true }); |
There was a problem hiding this comment.
Looks like at the moment the source/importer never change, so the skip logic being scoped to the source/importer combo might not have coverage?
There was a problem hiding this comment.
I cheated a little here. The secret sauce is in the lines below "to make this more interesting". But maybe I should actually assert the return values.
| @@ -697,7 +697,7 @@ Use Rollup's internal acorn instance to parse code to an AST. | |||
| #### `this.resolve(source: string, importer?: string, options?: {skipSelf?: boolean, custom?: {[plugin: string]: any}}) => Promise<{id: string, external: boolean, moduleSideEffects: boolean | 'no-treeshake', syntheticNamedExports: boolean | string, meta: {[plugin: string]: any}} | null>` | |||
| Resolve imports to module ids (i.e. file names) using the same plugins that Rollup uses, and determine if an import should be external. If `null` is returned, the import could not be resolved by Rollup or any plugin but was not explicitly marked as external by the user. | |||
| If you pass `skipSelf: true`, then the `resolveId` hook of the plugin from which `this.resolve` is called will be skipped when resolving. | |||
| If you pass `skipSelf: true`, then the `resolveId` hook of the plugin from which `this.resolve` is called will be skipped when resolving. When other plugins themselves also call `this.resolve` in their `resolveId` hooks with the *exact same `source` and `importer`* while handling the original `this.resolve` call, then the `resolveId` hook of the original plugin will be skipped for those calls as well. The rationale here is that the plugin already stated that it "does not know" how to resolve this particular combination of `source` and `importer` at this point in time. If you do not want this behaviour, do not use `skipSelf` but implement your own infinite loop prevention mechanism if necessary. | |||
There was a problem hiding this comment.
👍 still pretty confusing, but I'm not sure how it could be any clearer 😆
Does feel like it fits the responsibility of skipSelf.
This PR contains:
Are tests included?
Breaking Changes?
List any relevant issue numbers:
Description
This will change the logic when using
skipSelfinthis.resolve. With this patch, this will not only skip the current plugin for this call ofthis.resolvebut when one of the other plugins callsthis.resolveagain from its resolveId hook with the exact same source and importer, the initial plugin will be skipped as well.Example:
What would happen without this patch is that when a pair of (source, importer) is resolved, it is passed to the first plugin, which calls
this.resolveto pass it to the second plugin, which again callsthis.resolveto pass it back to the first plugin.With this patch, the pair of (source, importer) is passed to the first plugin, which calls
this.resolveto pass it to the second plugin (the first is skipped byskipSelf) and core. As the call ofthis.resolvein the second plugin uses the samesourceandimporteras the call the first plugin wanted skipped, this is skipping now both plugins, passing it directly to Rollup core.