Skip to content

Comments

fix: return moduleSideEffects instead of sideEffects from callable plugins#5206

Merged
sapphi-red merged 1 commit intomainfrom
07-08-fix_return_modulesideeffects_instead_of_sideeffects_from_callable_plugins
Jul 8, 2025
Merged

fix: return moduleSideEffects instead of sideEffects from callable plugins#5206
sapphi-red merged 1 commit intomainfrom
07-08-fix_return_modulesideeffects_instead_of_sideeffects_from_callable_plugins

Conversation

@sapphi-red
Copy link
Member

@sapphi-red sapphi-red commented Jul 8, 2025 •
edited
Loading

resolveId, load, transform hooks should return moduleSideEffects property instead of sideEffects property.
Callable plugins were returning sideEffects property.

This was referenced Jul 8, 2025
Merged
Merged
Merged
@sapphi-red Graphite App
Copy link
Member Author

sapphi-red commented Jul 8, 2025 •
edited
Loading

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

@sapphi-red sapphi-red marked this pull request as ready for review July 8, 2025 04:45
@github-actions
Copy link
Contributor

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

Benchmarks Rust

  • target: main(aa87300)
  • pr: 07-08-fix_return_modulesideeffects_instead_of_sideeffects_from_callable_plugins(25e2b0a)
group                                                        pr                                     target
-----                                                        --                                     ------
bundle/bundle@multi-duplicated-top-level-symbol              1.01     83.7±3.67ms        ? ?/sec    1.00     83.2±2.66ms        ? ?/sec
bundle/bundle@multi-duplicated-top-level-symbol-sourcemap    1.00     93.9±3.17ms        ? ?/sec    1.01     95.0±3.21ms        ? ?/sec
bundle/bundle@rome_ts                                        1.03    123.1±3.10ms        ? ?/sec    1.00    120.0±2.32ms        ? ?/sec
bundle/bundle@rome_ts-sourcemap                              1.00    141.0±2.82ms        ? ?/sec    1.00    140.6±2.30ms        ? ?/sec
bundle/bundle@threejs                                        1.00     44.5±1.10ms        ? ?/sec    1.00     44.5±1.04ms        ? ?/sec
bundle/bundle@threejs-sourcemap                              1.01     54.1±1.11ms        ? ?/sec    1.00     53.8±1.53ms        ? ?/sec
bundle/bundle@threejs10x                                     1.00    456.2±5.15ms        ? ?/sec    1.01    460.7±6.58ms        ? ?/sec
bundle/bundle@threejs10x-sourcemap                           1.00    537.8±8.11ms        ? ?/sec    1.00    535.2±6.47ms        ? ?/sec
scan/scan@rome_ts                                            1.00     93.8±1.72ms        ? ?/sec    1.01     94.4±1.85ms        ? ?/sec
scan/scan@threejs                                            1.00     33.4±1.92ms        ? ?/sec    1.00     33.4±1.99ms        ? ?/sec
scan/scan@threejs10x                                         1.00    336.9±3.75ms        ? ?/sec    1.01    341.4±5.84ms        ? ?/sec

Base automatically changed from 07-08-fix_return_modulesideeffects_set_by_native_plugins_from_this.resolve_ to main July 8, 2025 06:52
@sapphi-red sapphi-red force-pushed the 07-08-fix_return_modulesideeffects_instead_of_sideeffects_from_callable_plugins branch from dac67c0 to 25e2b0a Compare July 8, 2025 06:56
@graphite-app
Copy link
Contributor

graphite-app bot commented Jul 8, 2025

Merge activity

  • Jul 8, 6:56 AM UTC: Graphite rebased this pull request, because this pull request is set to merge when ready.

@netlify
Copy link

netlify bot commented Jul 8, 2025

✅ Deploy Preview for rolldown-rs ready!

Name Link
🔨 Latest commit 25e2b0a
🔍 Latest deploy log https://app.netlify.com/projects/rolldown-rs/deploys/686cc11fdea6ef0007f57f9d
😎 Deploy Preview https://deploy-preview-5206--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.

@sapphi-red sapphi-red added this pull request to the merge queue Jul 8, 2025
Merged via the queue into main with commit e6cbb01 Jul 8, 2025
22 checks passed
@sapphi-red sapphi-red deleted the 07-08-fix_return_modulesideeffects_instead_of_sideeffects_from_callable_plugins branch July 8, 2025 07:11
@github-actions github-actions bot mentioned this pull request Jul 10, 2025
Merged
IWANABETHATGUY pushed a commit that referenced this pull request Jul 10, 2025
@github-actions @shulaoda
release: v1.0.0-beta.25 (#5236)
af400d3 
## [1.0.0-beta.25] - 2025-07-10

### 🚀 Features

- hmr: should initialize cjs with correct initializer (#5234) by @hyf0
- hmr: handle `import(..)` on cjs modules (#5225) by @hyf0
- support `chunkModulesOrder`  (#5227) by @IWANABETHATGUY
- add `export`s to `rolldown:runtime` (#5224) by @hyf0
- hmr: support `import(..)` (#5223) by @hyf0
- add analyzed source code side effects field in ecmaMeta (#5226) by
@IWANABETHATGUY
- hmr: return `undefined` from `generateHmrPatch` when there is no patch
(#5218) by @sapphi-red
- pass `isEntry` to resolveId hook of CallableBuiltinPlugin (#5215) by
@sapphi-red
- pass `isEntry` from `this.resolve` (#5214) by @sapphi-red
- node: support `OutputOptions#minifyInternalExports` (#5210) by @hyf0
- rolldown: oxc v0.76.0 (#5212) by @Boshen
- rust: support `minify_internal_exports` (#5208) by @hyf0
- support opt-in `topLevelVar` (#5188) by @7086cmd
- rolldown: oxc-resolver v11.5.0 (yarn pnp) (#5147) by @Boshen

### 🐛 Bug Fixes

- do not optimize interop code for `mod.default` if mod bind from
default import (#5232) by @IWANABETHATGUY
- return `moduleSideEffects` instead of `sideEffects` from callable
plugins (#5206) by @sapphi-red
- return `moduleSideEffects` set by native plugins from `this.resolve`
(#5205) by @sapphi-red
- incorrect output when importing CJS package (#5201) by @IWANABETHATGUY
- use resolved id instead of module_request when analysis if a module is
a json module (#5200) by @IWANABETHATGUY

### 🚜 Refactor

- use `sort_unstable_by_key` for simple property access (#5233) by
@IWANABETHATGUY
- add Deref impl for TransformPluginContext to access PluginContext
(#5221) by @sapphi-red
- use `BindingHookSideEffects` in CallableBuiltinPlugin (#5204) by
@sapphi-red
- move side effects type bindings logic to rust side (#5203) by
@sapphi-red
- test: improve experience of writing manual integration test (#5186) by
@hyf0
- rust: unify logic of `IntegrationTest` (#5185) by @hyf0

### 🧪 Testing

- rust: automatically run extented test for `minify_internal_exports:
true` (#5213) by @hyf0
- rust: add basic tests for minifying internal exports (#5211) by @hyf0

### ⚙️ Miscellaneous Tasks

- deps: oxc-browserslist v2.0.10 and oxc-resolver v11.5.2 (#5222) by
@Boshen
- use jsonc schema for knip config (#5207) by @sapphi-red
- rust: remove unused code related to
`safe_cjs_to_eliminate_interop_default` (#5202) by @shulaoda
- inject test variant name to rust unit test script (#5195) by
@IWANABETHATGUY
- change CC => TARGET_CC (#5194) by @Brooooooklyn
- deps: lock file maintenance npm packages (#5191) by @renovate[bot]
- deps: lock file maintenance (#5193) by @renovate[bot]
- deps: lock file maintenance rust crates (#5192) by @renovate[bot]
- deps: update github-actions (#5190) by @renovate[bot]
- test: remove unnecessary test snapshots (#5183) by @shulaoda
- rolldown: upgrade to NAPI-RS 3.0.0-beta.12 (#5184) by @Brooooooklyn

Co-authored-by: shulaoda <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hyf0 hyf0 hyf0 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@sapphi-red @hyf0