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: replace runner-side path normalization with fetchModule-side resolve #18361

Merged
merged 42 commits into from
Dec 24, 2024

Conversation

hi-ogawa
Copy link
Collaborator

@hi-ogawa hi-ogawa commented Oct 16, 2024

Description

Not so confident that this is a proper fix nor the issue needs fix, but I'm thinking about what would require to support #18223 and hopefully it helps deciding the fate of the issue (bug or wontfix).

One thing I noticed that it looks possible to dedupe/normalize /abs-path-to/src/etc -> /src/etc on server side (fetchModule) and module identity is still kept intact on runner side since they are keyed by resolved id. For FetchModuleResult.cache detection, I replaced getModuleByUrl with ensureEntryFromUrl and I'm wondering what's the downside of this.

todo

Copy link

stackblitz bot commented Oct 16, 2024

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@hi-ogawa hi-ogawa force-pushed the fix-normalize-url-in-fetchModule branch from bb8c402 to 5e8e7ec Compare October 16, 2024 04:24
@hi-ogawa hi-ogawa changed the title fix(runner): normalize absolute path in fetchModule fix(runner): resolve and normalize absolute path in fetchModule Oct 16, 2024
@hi-ogawa
Copy link
Collaborator Author

normalizeAbsoluteUrl also noramlizes file:///... to support this test case, but I'm not sure if we needs to keep this if we go with externalizing file:// entirely #17369. Also v5's ssrLoadModule('file://') doesn't seem to work either.

// loads the same module if id is a file url
const fileUrl = new _URL('./fixtures/simple.js', import.meta.url)
const mod2 = await runner.import(fileUrl.toString())
expect(mod).toBe(mod2)

@hi-ogawa hi-ogawa marked this pull request as ready for review October 16, 2024 07:06
@hi-ogawa

This comment was marked as outdated.

@vite-ecosystem-ci

This comment was marked as outdated.

@hi-ogawa hi-ogawa marked this pull request as draft October 16, 2024 23:12
@hi-ogawa

This comment was marked as outdated.

@vite-ecosystem-ci

This comment was marked as outdated.

@hi-ogawa hi-ogawa changed the title fix(runner): resolve and normalize absolute path in fetchModule fix(runner): move path normalization from runner to fetchModule Oct 21, 2024
@hi-ogawa hi-ogawa changed the title fix(runner): move path normalization from runner to fetchModule fix(runner): replace runner path normalization with fetchModule resolve Oct 21, 2024
@hi-ogawa hi-ogawa force-pushed the fix-normalize-url-in-fetchModule branch from 272e729 to 06d820e Compare December 10, 2024 01:10
@hi-ogawa
Copy link
Collaborator Author

/ecosystem-ci run

@vite-ecosystem-ci
Copy link

@hi-ogawa hi-ogawa added the p2-to-be-discussed Enhancement under consideration (priority) label Dec 10, 2024
Comment on lines 183 to 187
/**
* Root of the project
* @deprecated not used anymore and to be removed
*/
root: string
root?: string
Copy link
Member

Choose a reason for hiding this comment

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

Since root is deprecated and does not need to be set anymore, I think we can remove root from all the examples and types in the docs.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Makes sense. Updated the docs and also removed the internal usages of root.
I kept the default server runner one since users can technically access it via runner.options.root.

dai-shi added a commit to himself65/waku that referenced this pull request Dec 11, 2024
@sapphi-red
Copy link
Member

@hi-ogawa Do you think this can be released in a patch? or do you prefer it to be released in a minor to be safe?

@hi-ogawa
Copy link
Collaborator Author

I think we can go as patch. runner.options.root becoming optional is actually a breaking change, but people who potentially consume that is only a few people, so I guess it's alright. Other than that, this should be a reasonable fix and I need to follow up on that if this breaks anyways.

@dai-shi
Copy link

dai-shi commented Dec 20, 2024

Hi there, can I get an estimate about the release time?

@sapphi-red sapphi-red merged commit 9f10261 into vitejs:main Dec 24, 2024
17 checks passed
@sapphi-red
Copy link
Member

@dai-shi Probably I'll cut a release in few days 🫡

@hi-ogawa hi-ogawa deleted the fix-normalize-url-in-fetchModule branch December 24, 2024 06:01
renovate bot added a commit to andrei-picus-tink/auto-renovate that referenced this pull request Dec 26, 2024
| datasource | package | from  | to    |
| ---------- | ------- | ----- | ----- |
| npm        | vite    | 6.0.5 | 6.0.6 |


## [v6.0.6](https://github.com/vitejs/vite/blob/HEAD/packages/vite/CHANGELOG.md#small606-2024-12-26-small)

-   fix: replace runner-side path normalization with `fetchModule`-side resolve ([#18361](vitejs/vite#18361)) ([9f10261](vitejs/vite@9f10261)), closes [#18361](vitejs/vite#18361)
-   fix(css): resolve style tags in HTML files correctly for lightningcss ([#19001](vitejs/vite#19001)) ([afff05c](vitejs/vite@afff05c)), closes [#19001](vitejs/vite#19001)
-   fix(css): show correct error when unknown placeholder is used for CSS modules pattern in lightningcs ([9290d85](vitejs/vite@9290d85)), closes [#19070](vitejs/vite#19070)
-   fix(resolve): handle package.json with UTF-8 BOM ([#19000](vitejs/vite#19000)) ([902567a](vitejs/vite@902567a)), closes [#19000](vitejs/vite#19000)
-   fix(ssrTransform): preserve line offset when transforming imports ([#19004](vitejs/vite#19004)) ([1aa434e](vitejs/vite@1aa434e)), closes [#19004](vitejs/vite#19004)
-   chore: fix typo in comment ([#19067](vitejs/vite#19067)) ([eb06ec3](vitejs/vite@eb06ec3)), closes [#19067](vitejs/vite#19067)
-   chore: update comment about `build.target` ([#19047](vitejs/vite#19047)) ([0e9e81f](vitejs/vite@0e9e81f)), closes [#19047](vitejs/vite#19047)
-   revert: unpin esbuild version ([#19043](vitejs/vite#19043)) ([8bfe247](vitejs/vite@8bfe247)), closes [#19043](vitejs/vite#19043)
-   test(ssr): test virtual module with query ([#19044](vitejs/vite#19044)) ([a1f4b46](vitejs/vite@a1f4b46)), closes [#19044](vitejs/vite#19044)
dai-shi added a commit to himself65/waku that referenced this pull request Dec 27, 2024
dai-shi added a commit to himself65/waku that referenced this pull request Dec 27, 2024
dai-shi added a commit to dai-shi/waku that referenced this pull request Dec 27, 2024
Blocking issues:

- [x] `polyfillModulePreload is deprecated. Use modulePreload.polyfill
instead. (x6)`
- [x] rscRsdwPlugin parse code is somehow incorrect, I think the code it
receives is already a bundlered code, that's why `const index =
code.indexOf('function requireAsyncModule(id)');` will get `-1`
- [x] vite issue. vitejs/vite#18361 
- [x] no styling in dev. 21_create-pages_standalone spec fails.
- [x] react-error-boundary. ssr-catch-error spec fails.

Other issues:

- [x] proper fix for `resolve.conditions` hack
- [x] proper fix for `vite-loader.ts` hack
- [ ] drop `viteServer.hot`.
#1012 (comment)

---------

Co-authored-by: daishi <daishi@axlight.com>
Co-authored-by: Daishi Kato <dai-shi@users.noreply.github.com>
Co-authored-by: Tyler <26290074+thegitduck@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p2-to-be-discussed Enhancement under consideration (priority) trigger: preview
Projects
Status: Approved
Development

Successfully merging this pull request may close these issues.

virtual file system is broken in v6
4 participants