fix: forward options when calling this.resolve
#128
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
aleclarson
reviewed
Dec 7, 2023
| if (importer && !relativeImportRE.test(id) && !isAbsolute(id)) { | ||
| const viteResolve: ViteResolve = async (id, importer) => | ||
| (await this.resolve(id, importer, { skipSelf: true }))?.id | ||
| (await this.resolve(id, importer, options))?.id |
There was a problem hiding this comment.
It still needs skipSelf: true I think?
There was a problem hiding this comment.
From the Vite docs for this.resolve:
The default of
skipSelfistrue
But no harm (I think?) in setting it explicitly too if you prefer.
There was a problem hiding this comment.
Oh nice. That's a welcome change from Rollup :)
There was a problem hiding this comment.
Isn't this a breaking changes for people still using Vite 3-4?
Rollup changed the default into v4, which is used by Vite 5, thus this is a breaking change and would need a proper major release
There was a problem hiding this comment.
@IlCallo Ah good point! I think I'll explicitly set skipSelf: true for backwards compatibility.
|
Released in v4.2.2 |
|
@aleclarson so swift! 🙏 |
aleclarson
added a commit
that referenced
this pull request
Dec 28, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently, passing only
skipSelfand not forwardingoptionscausescustomto be set toundefined, but other plugins rely on the presence ofcustomfor correctness.For example, Remix would like to throw whenever
.servermodules are resolved from the client bundle. We'd like to show something like:We need the resolved ID to check if the actual file path contains
.server, but we need the original ID for the error message. So we callthis.resolveto get the resolved ID. To make sure our plugin doesn't participate in further resolution, we rely oncustomto setup some custom logic. But sincevite-tsconfig-pathsis also used in this project and it setsoptionswithcustomasundefinedimplicitly, the data set incustomis lost and our custom logic never triggers.Additionally, omitting
optionsaltogether results in this warning from@rollup/plugin-commonjs:Since they can't reliably check that you forwarded the options, they just check that
optionswas passed in at all, but its clear from the message that the intent is to forwardoptions.