Skip to content

fix: ensure resource data always have resource path#11632

Merged
ahabhgk merged 7 commits intomainfrom
fix-ensure-resource-path
Sep 11, 2025
Merged

fix: ensure resource data always have resource path#11632
ahabhgk merged 7 commits intomainfrom
fix-ensure-resource-path

Conversation

@ahabhgk
Copy link
Contributor

@ahabhgk ahabhgk commented Sep 10, 2025 •
edited
Loading

Summary

Ensure resource data always have resource path

  • split ResourceData::new to ResourceData::new_with_path and ResourceData::new_with_resource, you should use new_with_path when you already have the path, query, and fragment, otherwise you should use new_with_resource, which will calculate path, query, and fragment automatically
  • refactor methods on ResourceData, make fields on ResourceData private

fix #11513

Related links

Checklist

  • Tests updated (or not required).
  • Documentation updated (or not required).

Copilot AI review requested due to automatic review settings September 10, 2025 11:08
@ahabhgk ahabhgk requested a review from h-a-n-a as a code owner September 10, 2025 11:08
@netlify
Copy link

netlify bot commented Sep 10, 2025 •
edited
Loading

✅ Deploy Preview for rspack canceled.

Name Link
🔨 Latest commit 9f1df4d
🔍 Latest deploy log https://app.netlify.com/projects/rspack/deploys/68c272d9614c750008d6c166

@github-actions github-actions bot added the team The issue/pr is created by the member of Rspack. label Sep 10, 2025
@ahabhgk ahabhgk requested review from SyMind and removed request for Copilot and h-a-n-a September 10, 2025 11:08
@github-actions github-actions bot added the release: bug fix release: bug related release(mr only) label Sep 10, 2025
@ahabhgk ahabhgk enabled auto-merge (squash) September 10, 2025 11:09
Copilot AI review requested due to automatic review settings September 10, 2025 11:17
Copilot AI reviewed Sep 10, 2025
View reviewed changes
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR ensures that resource data always has a resource path to fix issue #11513. The change refactors the ResourceData construction to always include path information, replacing the builder pattern with dedicated constructors.

  • Introduces new_with_resource and new_with_path constructors for ResourceData
  • Updates all ResourceData creation sites to use the new constructors
  • Adds test coverage for resolveForScheme hook to verify resource path is properly set

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
crates/rspack_loader_runner/src/content.rs Adds new constructors new_with_resource and new_with_path to ResourceData
crates/rspack_core/src/resolver/mod.rs Updates Resource to ResourceData conversion to use new constructor
crates/rspack_core/src/normal_module_factory.rs Replaces ResourceData::new calls with new constructors throughout
crates/rspack_plugin_schemes/src/file_uri.rs Simplifies file URI handling using new_with_path constructor
crates/rspack_binding_api/src/modules/normal_module.rs Updates match resource creation to use new_with_path
tests/rspack-test/configCases/hooks/resolve-for-scheme/ Adds test to verify resource path is correctly set in resolveForScheme hook
crates/rspack_loader_swc/src/lib.rs Removes unnecessary blank line

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@ahabhgk ahabhgk requested a review from h-a-n-a September 10, 2025 11:18
@github-actions
Copy link
Contributor

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

📦 Binary Size-limit

Comparing 9f1df4d to fix: persistent cache update issuer is updated modules (#11633) by jinrui

🎉 Size decreased by 128bytes from 47.23MB to 47.23MB (⬇️0.00%)

SyMind
SyMind previously approved these changes Sep 10, 2025
View reviewed changes
@codspeed-hq
Copy link

codspeed-hq bot commented Sep 10, 2025 •
edited
Loading

CodSpeed Performance Report

Merging #11632 will not alter performance

Comparing fix-ensure-resource-path (9f1df4d) with main (d005492)1

Summary

✅ 17 untouched benchmarks

Footnotes

  1. No successful run was found on main (195ce01) during the generation of this report, so d005492 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩

@ahabhgk ahabhgk requested a review from SyMind September 11, 2025 06:03
@ahabhgk ahabhgk merged commit 8d5812f into main Sep 11, 2025
68 of 70 checks passed
@ahabhgk ahabhgk deleted the fix-ensure-resource-path branch September 11, 2025 07:27
@hardfist
Copy link
Contributor

hardfist commented Sep 18, 2025

@codex review

@chatgpt-codex-connector
Copy link

chatgpt-codex-connector bot commented Sep 18, 2025

Codex Review: Here are some suggestions.

rspack/crates/rspack_loader_runner/src/content.rs

Lines 212 to 219 in 8d5812f

pub fn set_path<P: Into<Utf8PathBuf>>(&mut self, v: P) {
let new_path = v.into();
if let Some(path) = self.path()
&& path != new_path
{
self.scheme.take();
self.resource_path = Some(new_path);
}

[P1] Allow set_path to populate empty resource_path

The new ResourceData::set_path assigns the new path only when self.path() returns Some and differs from the incoming value. When resource_path is currently None—which happens for resources that failed to parse and is the very case this patch wants to cover—the condition is skipped entirely, leaving resource_path unset and the cached scheme unchanged. Any caller attempting to set a path on an initially pathless resource will silently fail, so downstream logic will still treat the resource as lacking a path and will not recalculate the scheme. Move the self.resource_path = Some(new_path) outside of the conditional and only guard the scheme.take() by the equality check.

Reply with @codex fix comments to fix any unresolved comments.

About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you open a pull request for review, mark a draft as ready, or comment "@codex review". If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex fix this CI failure" or "@codex address that feedback".

@slorber
Copy link

slorber commented Sep 19, 2025

This may be the cause of this bug we now have starting 1.5.4: #11717

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@SyMind SyMind SyMind approved these changes

@h-a-n-a h-a-n-a Awaiting requested review from h-a-n-a

@LingyuCoder LingyuCoder Awaiting requested review from LingyuCoder LingyuCoder is a code owner

Assignees

No one assigned

Labels

release: bug fix release: bug related release(mr only) team The issue/pr is created by the member of Rspack.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Bug]: an empty path key gets added to window.__coverage__ when ModuleFedereationPlugin is used. This causes an error when processing instrumented paths

5 participants

@ahabhgk @hardfist @slorber @SyMind