Skip to content

Conversation

sapphi-red
Copy link
Member

@sapphi-red sapphi-red commented May 27, 2025

Description

According to #2519 and e6f7fba#diff-c21c6724d412c50eabaaaf67a8fa37cea7ab889f0c23e628713893f7dd34770a, it seems the isInNodeModules(importer) && condition was to skip import.meta.glob processing in deps.
But now that import.meta.glob processing is done in a separate plugin, this condition no longer made sense.

The current condition is skipping edge case dynamic imports only for deps (example cases of them - es-module-lexer repl).

I think the current code is difficult to understand the intent and also the behavior is strange. I think no one would write these in the source code, so I guess we can just remove this condition. If we should support these, we can update the dynamicImportPrefixRE regex to /import\s*[(/]/.

@sapphi-red sapphi-red added the p2-edge-case Bug, but has workaround or limited in scope (priority) label May 27, 2025
@patak-dev
Copy link
Member

/ecosystem-ci run

Copy link

pkg-pr-new bot commented May 27, 2025

Open in StackBlitz

npm i https://pkg.pr.new/vite@20115

commit: 15d2a3f

@vite-ecosystem-ci
Copy link

📝 Ran ecosystem CI on 75521eb: Open

suite result latest scheduled
histoire failure success
analogjs failure failure
previewjs failure success
qwik failure success
laravel failure failure
nuxt success failure
waku failure success
vike failure success
vitest failure failure

ladle, marko, quasar, rakkas, astro, react-router, vite-plugin-react, vuepress, vite-environment-examples, vite-plugin-pwa, vitepress, unocss, storybook, vite-setup-catalogue, vite-plugin-cloudflare, sveltekit, vite-plugin-vue, vite-plugin-svelte

@patak-dev
Copy link
Member

It seems that #19936 will cause some regressions in the ecosystem. For code like this one https://github.com/wakujs/waku/blob/16f21c7ce460071392e995a634acc058a7c5ed53/packages/waku/src/lib/plugins/patch-react-refresh.ts#L14 that calls transformIndexHtml manually. Maybe we need to accept these issues. I don't know if there is a way to avoid them.

@sapphi-red
Copy link
Member Author

Maybe we need to accept these issues. I don't know if there is a way to avoid them.

Yeah, I think we cannot avoid these.

@sapphi-red
Copy link
Member Author

/ecosystem-ci run

@vite-ecosystem-ci
Copy link

📝 Ran ecosystem CI on 15d2a3f: Open

suite result latest scheduled
analogjs failure failure
astro failure failure
histoire failure failure
previewjs failure failure
sveltekit failure failure
vike failure failure
vite-plugin-cloudflare failure failure

ladle, laravel, qwik, nuxt, rakkas, react-router, quasar, marko, vitepress, vite-plugin-svelte, vite-setup-catalogue, vite-plugin-vue, vitest, vite-plugin-react, unocss, storybook, waku, vite-plugin-pwa, vuepress, vite-environment-examples

@sapphi-red
Copy link
Member Author

vike is failing with a different error but the reason is same with #20080 (comment) and not caused by this PR.

@sapphi-red sapphi-red merged commit 1ea2222 into vitejs:main Jun 6, 2025
16 checks passed
@sapphi-red sapphi-red deleted the fix/align-dynamic-import-detection branch June 6, 2025 08:42
shulaoda added a commit to rolldown/rolldown that referenced this pull request Jun 19, 2025
github-merge-queue bot pushed a commit to rolldown/rolldown that referenced this pull request Jun 19, 2025
hyf0 pushed a commit to rolldown/rolldown that referenced this pull request Jun 22, 2025
## [1.0.0-beta.19] - 2025-06-22

### 🚀 Features

- support `OutputOptions#manualChunks` (#5037) by @hyf0
- advanced-chunks: support `advancedChunks#gruop#name` to be function
(#5035) by @hyf0
- rolldown_plugin_import_glob: align with `vitejs/vite#20163` (#5034) by
@shulaoda
- rust/advanced-chunks: support `MatchGroup#name` to be dynamic (#5033)
by @hyf0
- rolldown_plugin_build_import_analysis: align with `vitejs/vite#20117`
(#5027) by @shulaoda
- rolldown_plugin_build_import_analysis: align with `vitejs/vite#20115`
(#5020) by @shulaoda
- add validation warning for advanced chunks options without groups
(#5009) by @sapphi-red

### 🐛 Bug Fixes

- moduleInfo is not updated when entry module is emitted by
this.emitFile (#5032) by @IWANABETHATGUY
- preserveEntrySignatures: false generates circular imports that hangs
with TLA (#5029) by @IWANABETHATGUY
- rolldown_plugin_build_import_analysis: align pure dynamic import
handling with rolldown-vite (#5016) by @shulaoda
- plugin/vite-resolve: normalize leading slash (#5013) by @sapphi-red
- debug: `build_id` doesn't increase (#5015) by @hyf0
- side effects in this.emitFile({ type: 'chunk' }) is removed when
preserveEntrySignatures: false is set (#5012) by @IWANABETHATGUY

### 🚜 Refactor

- rolldown_utils: simplify `block_on_spawn_all` (#5019) by @shulaoda
- use `rolldown_utils::futures::block_on` for `WatcherImpl#start`
(#5018) by @shulaoda

### 📚 Documentation

- jsdoc: document `experimental.attachDebugInfo` (#5028) by @hyf0
- clarify that `advancedChunks` options are in bytes (#5022) by
@sapphi-red
- add a note that sequential conversion may break the code (#5024) by
@sapphi-red

### ⚙️ Miscellaneous Tasks

- infra: clean up `dist` before building `rolldown` (#5036) by @hyf0
- Align status notice in readme with documentation (#5021) by
@rijkvanzanten

### ❤️ New Contributors

* @rijkvanzanten made their first contribution in
[#5021](#5021)

Co-authored-by: shulaoda <165626830+shulaoda@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p2-edge-case Bug, but has workaround or limited in scope (priority) trigger: preview
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants