Skip to content

fix(rolldown_plugin_vite_resolve): align bare import logic#5247

Closed
shulaoda wants to merge 1 commit intomainfrom
07-11-fix_rolldown_plugin_vite_resolve_align_bare_import_logic
Closed

fix(rolldown_plugin_vite_resolve): align bare import logic#5247
shulaoda wants to merge 1 commit intomainfrom
07-11-fix_rolldown_plugin_vite_resolve_align_bare_import_logic

Conversation

@shulaoda
Copy link
Member

@shulaoda shulaoda commented Jul 10, 2025 •
edited
Loading

closes vitejs/rolldown-vite#288

Since I'm not very familiar with the related logic, I provided a quick workaround. We don't necessarily have to merge it — it's just to help you understand where the issue lies. @sapphi-red cc

The root cause is that we didn't align with the logic in resolve.ts. Specifically, we're missing the tryResolveBrowserMapping and tryNodeResolve logic. In this issue, it hits the tryNodeResolve branch, which leads to the problem.

The integration of rolldown-vite is as follows:

async finalizeBareSpecifier(id, importer, scan) {
  const environment = getEnv()
  const external =
    options.externalize &&
    options.isBuild &&
    partialEnv.config.consumer === 'server' &&
    shouldExternalize(getEnv(), id, importer)

  const depsOptimizer =
    resolveOptions.optimizeDeps && environment.mode === 'dev'
      ? environment.depsOptimizer
      : undefined

  let res: string | PartialResolvedId | undefined
  if (
    !external &&
    options.asSrc &&
    depsOptimizer &&
    !scan &&
    (res = await tryOptimizedResolve(
      depsOptimizer,
      id,
      importer,
      options.preserveSymlinks,
      options.packageCache,
    ))
  ) {
    return res
  }

  if (
    options.mainFields.includes('browser') &&
    (res = tryResolveBrowserMapping(
      id,
      importer,
      options,
      false,
      external,
    ))
  ) {
    return res
  }

  if (
    (res = tryNodeResolve(
      id,
      importer,
      options,
      depsOptimizer,
      external,
    ))
  ) {
    return res
  }
}

@shulaoda Graphite App
Copy link
Member Author

shulaoda commented Jul 10, 2025

This stack of pull requests is managed by Graphite. Learn more about stacking.

@netlify
Copy link

netlify bot commented Jul 10, 2025 •
edited
Loading

✅ Deploy Preview for rolldown-rs ready!

Name Link
🔨 Latest commit f74a8ec
🔍 Latest deploy log https://app.netlify.com/projects/rolldown-rs/deploys/686ffe933cdba9000898249c
😎 Deploy Preview https://deploy-preview-5247--rolldown-rs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@shulaoda shulaoda requested a review from sapphi-red July 10, 2025 17:40
@shulaoda shulaoda mentioned this pull request Jul 10, 2025
Closed
6 tasks
@github-actions
Copy link
Contributor

github-actions bot commented Jul 10, 2025 •
edited
Loading

Benchmarks Rust

  • target: main(79d6dc7)
  • pr: 07-11-fix_rolldown_plugin_vite_resolve_align_bare_import_logic(f74a8ec)
group                                                        pr                                     target
-----                                                        --                                     ------
bundle/bundle@multi-duplicated-top-level-symbol              1.00     77.8±2.00ms        ? ?/sec    1.00     77.8±3.42ms        ? ?/sec
bundle/bundle@multi-duplicated-top-level-symbol-sourcemap    1.01     89.1±1.46ms        ? ?/sec    1.00     88.4±1.68ms        ? ?/sec
bundle/bundle@rome_ts                                        1.04    118.3±3.65ms        ? ?/sec    1.00    114.2±1.84ms        ? ?/sec
bundle/bundle@rome_ts-sourcemap                              1.00    135.1±1.99ms        ? ?/sec    1.01    135.8±2.27ms        ? ?/sec
bundle/bundle@threejs                                        1.02     42.9±1.56ms        ? ?/sec    1.00     42.0±1.02ms        ? ?/sec
bundle/bundle@threejs-sourcemap                              1.01     51.8±1.03ms        ? ?/sec    1.00     51.4±0.73ms        ? ?/sec
bundle/bundle@threejs10x                                     1.00    441.7±5.67ms        ? ?/sec    1.00    441.7±4.72ms        ? ?/sec
bundle/bundle@threejs10x-sourcemap                           1.01    517.5±3.57ms        ? ?/sec    1.00    511.2±3.48ms        ? ?/sec
scan/scan@rome_ts                                            1.00     88.7±1.62ms        ? ?/sec    1.01     89.7±5.71ms        ? ?/sec
scan/scan@threejs                                            1.02     31.1±2.11ms        ? ?/sec    1.00     30.5±0.36ms        ? ?/sec
scan/scan@threejs10x                                         1.00    322.8±3.95ms        ? ?/sec    1.01    324.6±5.14ms        ? ?/sec

@shulaoda shulaoda force-pushed the 07-11-fix_rolldown_plugin_vite_resolve_align_bare_import_logic branch from 511caf4 to f74a8ec Compare July 10, 2025 17:55
@sapphi-red
Copy link
Member

sapphi-red commented Jul 11, 2025

tryResolveBrowserMapping and tryNodeResolve are handled inside oxc resolver. There are few edge case differences, but it's almost the same.

What was the concrete resolution difference? Which id + importer was resolved to what previously and what will be resolved to, by this PR?

@shulaoda
Copy link
Member Author

shulaoda commented Jul 11, 2025 •
edited by sapphi-red
Loading

tryResolveBrowserMapping and tryNodeResolve are handled inside oxc resolver. There are few edge case differences, but it's almost the same.

What was the concrete resolution difference? Which id + importer was resolved to what previously and what will be resolved to, by this PR?

  1. git clone https://github.com/YLMC2017/rolldown-vite-test
  2. set enableNativePlugin to 'resolver'
  3. debug at

    rolldown/crates/rolldown_plugin/src/plugin_driver/build_hooks.rs

    Line 131 in 9111a72

    if ret.is_some() { 
if ret.is_some() {
  if args.specifier == "element-plus/es/components/button/style/css" {
    println!("{:?} {:?}", args.importer, ret);
  }
  return Ok(ret);
}

In both of the following results, side_effects should be True.

specifier: "element-plus/es/components/button/style/css"
importer: "D:/shulaoda/rolldown-vite-test/packages/app/src/App.vue",

ret -> HookResolveIdOutput {
    id: "D:/shulaoda/rolldown-vite-test/node_modules/.pnpm/element-plus@2.10.2_vue@3.5.17/node_modules/element-plus/es/components/button/style/css.mjs",
    external: None,
    normalize_external_id: None,
    side_effects: Some(
        False,
    ),
}
specifier: "element-plus/es/components/button/style/css"
importer: "D:/shulaoda/rolldown-vite-test/packages/app/src/login.vue",

ret -> HookResolveIdOutput {
    id: "D:/shulaoda/rolldown-vite-test/node_modules/.pnpm/element-plus@2.10.2_vue@3.5.17/node_modules/element-plus/es/components/button/style/css.mjs",
    external: None,
    normalize_external_id: None,
    side_effects: Some(
        False,
    ),
}

@sapphi-red
Copy link
Member

sapphi-red commented Jul 11, 2025

It seems the sideEffects field was not handled correctly. I made a fix at #5250

@shulaoda shulaoda mentioned this pull request Jul 11, 2025
Merged
github-merge-queue bot pushed a commit that referenced this pull request Jul 11, 2025
@sapphi-red @shulaoda
fix: normalize path from package.json's realpath before matching side…
ba12fc8 
…Effects field (#5250)

closes vitejs/rolldown-vite#288, #5247

Co-authored-by: shulaoda <165626830+shulaoda@users.noreply.github.com>
@shulaoda shulaoda closed this Jul 11, 2025
@shulaoda shulaoda deleted the 07-11-fix_rolldown_plugin_vite_resolve_align_bare_import_logic branch July 11, 2025 06:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sapphi-red sapphi-red Awaiting requested review from sapphi-red

Copilot code review Copilot Awaiting requested review from Copilot Copilot will automatically review once the pull request is marked ready for review

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

pnpm monorepo project, ElementPlus css is missing when enableNativePlugin in production

2 participants

@shulaoda @sapphi-red