Skip to content

Conversation

@stormslowly
Copy link
Contributor

@stormslowly stormslowly commented Jan 7, 2026

Summary

Remove Default trait of DerefOption

  1. default to Some(T) is not a idomatic way in Rust
  2. Prevent DerefOption be mem::take, to make sure artifact is None after token.

Introduce take/replace paired API to transfer artifact in/out

Related links

Checklist

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

Copilot AI review requested due to automatic review settings January 7, 2026 09:39
@netlify
Copy link

netlify bot commented Jan 7, 2026

Deploy Preview for rspack canceled.

Name Link
🔨 Latest commit 062cd71
🔍 Latest deploy log https://app.netlify.com/projects/rspack/deploys/695f7e60673911000842d64b

@github-actions github-actions bot added release: refactor team The issue/pr is created by the member of Rspack. labels Jan 7, 2026
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 refactors the handling of build_module_graph_artifact to use the DerefOption::take() method instead of std::mem::take(), attempting to make the code more consistent with the custom DerefOption API. However, the PR introduces a critical bug in how the value is restored.

  • Replaced mem::take() with DerefOption::take() for extracting the artifact value
  • Attempted to use swap() method for restoring the artifact (incorrect approach)

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@github-actions
Copy link
Contributor

github-actions bot commented Jan 7, 2026

📦 Binary Size-limit

Comparing 062cd71 to feat: add constructor name for JS runtime modules (#12673) by harpsealjs

❌ Size increased by 128bytes from 47.87MB to 47.87MB (⬆️0.00%)

@stormslowly stormslowly marked this pull request as draft January 7, 2026 10:08
@github-actions
Copy link
Contributor

github-actions bot commented Jan 7, 2026

Rsdoctor Bundle Diff Analysis

Found 5 projects in monorepo, 0 projects with changes.

📊 Quick Summary
Project Total Size Change
react-10k 5.7 MB 0
react-1k 825.4 KB 0
react-5k 2.7 MB 0
rome 984.3 KB 0
ui-components 2.1 MB 0

Generated by Rsdoctor GitHub Action

@stormslowly stormslowly force-pushed the chore/test_artifact_take branch from 507c8ca to 644b566 Compare January 7, 2026 17:05
@codspeed-hq
Copy link

codspeed-hq bot commented Jan 7, 2026

Merging this PR will not alter performance

Summary

✅ 16 untouched benchmarks
⏩ 1 skipped benchmark1


Comparing chore/test_artifact_take (062cd71) with main (2feb455)

Open in CodSpeed

Footnotes

  1. 1 benchmark was skipped, so the baseline result was used instead. If it was deleted from the codebase, click here and archive it to remove it from the performance reports.

@stormslowly stormslowly force-pushed the chore/test_artifact_take branch from 9099eb9 to 2a408af Compare January 8, 2026 09:02
@stormslowly stormslowly changed the title refactor: use take() and swap() for DerefOpt refactor: use DerfOpt's take/replace assist artifact mutation Jan 8, 2026
@stormslowly stormslowly force-pushed the chore/test_artifact_take branch from 2a408af to 6e1cfd2 Compare January 8, 2026 09:13
@stormslowly stormslowly requested a review from hardfist January 8, 2026 09:52
@stormslowly stormslowly marked this pull request as ready for review January 8, 2026 09:52
@stormslowly stormslowly enabled auto-merge (squash) January 8, 2026 09:53
@stormslowly stormslowly merged commit 29642b2 into main Jan 8, 2026
102 of 106 checks passed
@stormslowly stormslowly deleted the chore/test_artifact_take branch January 8, 2026 13:08
LingyuCoder pushed a commit that referenced this pull request Jan 9, 2026
* refactor: remove Default trait of DerefOption
* refactor: `replace` is better than `insert`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release: refactor team The issue/pr is created by the member of Rspack.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants