Skip to content

Commit

Permalink
fix(treeshaking): spreading an expression is consider having side eff…
Browse files Browse the repository at this point in the history
…ect (#509)

<!-- Thank you for contributing! -->

### Description

<!-- Please insert your description here and provide especially info about the "what" this PR is solving -->

### Test Plan

<!-- e.g. is there anything you'd like reviewers to focus on? -->

---
  • Loading branch information
hyf0 committed Mar 9, 2024
1 parent 0a0eabc commit d094d56
Show file tree
Hide file tree
Showing 8 changed files with 61 additions and 19 deletions.
6 changes: 4 additions & 2 deletions crates/rolldown/src/ast_scanner/side_effect_detector.rs
Original file line number Diff line number Diff line change
Expand Up @@ -123,8 +123,10 @@ impl<'a> SideEffectDetector<'a> {

key_side_effect || prop_init_side_effect || value_side_effect
}
oxc::ast::ast::ObjectPropertyKind::SpreadProperty(spread) => {
self.detect_side_effect_of_expr(&spread.argument)
oxc::ast::ast::ObjectPropertyKind::SpreadProperty(_) => {
// ...[expression] is considered as having side effect.
// see crates/rolldown/tests/fixtures/rollup/object-spread-side-effect
true
}
})
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
---
source: crates/rolldown/tests/common/case.rs
expression: content
input_file: crates/rolldown/tests/fixtures/rollup/object-spread-side-effect
---
# Assets

## main.mjs

```js
import { default as assert } from "assert";
// main.js
let result = 'FAIL';
const unused = {
...{
get prop(){
result = 'PASS';
}
}
};
assert.strictEqual(result, 'PASS');
```
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
import assert from 'assert';
let result = 'FAIL';
const unused = {
...{
get prop() {
result = 'PASS';
}
}
};
assert.strictEqual(result, 'PASS');
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"input": {
"external": [
"assert"
]
}
}
20 changes: 10 additions & 10 deletions crates/rolldown_binding/index.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,11 @@ export interface SourceMap {
sources: Array<string>
sourcesContent: Array<string>
}
export interface OutputChunk {
export interface BindingOutputAsset {
fileName: string
source: string
}
export interface BindingOutputChunk {
isEntry: boolean
isDynamicEntry: boolean
facadeModuleId?: string
Expand All @@ -105,20 +109,16 @@ export interface OutputChunk {
modules: Record<string, BindingRenderedModule>
code: string
}
export interface OutputAsset {
fileName: string
source: string
}
export interface Outputs {
chunks: Array<OutputChunk>
assets: Array<OutputAsset>
export interface BindingOutputs {
chunks: Array<BindingOutputChunk>
assets: Array<BindingOutputAsset>
}
export interface BindingRenderedModule {
code?: string
}
export class Bundler {
constructor(inputOpts: InputOptions)
write(opts: OutputOptions): Promise<Outputs>
generate(opts: OutputOptions): Promise<Outputs>
write(opts: OutputOptions): Promise<BindingOutputs>
generate(opts: OutputOptions): Promise<BindingOutputs>
scan(): Promise<void>
}
2 changes: 1 addition & 1 deletion crates/rolldown_binding/src/options/input_options/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ impl From<InputOptions>
input: value.input.into_iter().map(Into::into).collect::<Vec<_>>(),
cwd,
external,
treeshake: false,
treeshake: true,
resolve: value.resolve.map(Into::into),
}),
value.plugins.into_iter().map(JsAdapterPlugin::new_boxed).collect::<napi::Result<Vec<_>>>(),
Expand Down
2 changes: 1 addition & 1 deletion packages/node/src/options/create-build-plugin-adapter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import type {
ResolveIdResult,
RenderedChunk,
HookRenderChunkOutput,
Outputs,
BindingOutputs as Outputs,
} from '@rolldown/node-binding'
import { transformToOutputBundle, unimplemented, transformSourcemap } from '../utils'

Expand Down
10 changes: 5 additions & 5 deletions packages/node/src/utils/transform-to-rollup-output.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
import { OutputAsset, OutputChunk, Outputs } from '@rolldown/node-binding'
import { RolldownOutput, RolldownOutputAsset, RolldownOutputChunk } from '../objects/rolldown-output'
import { OutputBundle } from '../objects/output-bundle'
import { BindingOutputAsset, BindingOutputChunk, BindingOutputs } from '@rolldown/node-binding'

function transformToRollupOutputChunk(chunk: OutputChunk): RolldownOutputChunk {
function transformToRollupOutputChunk(chunk: BindingOutputChunk): RolldownOutputChunk {
return {
type: 'chunk',
code: chunk.code,
Expand All @@ -17,15 +17,15 @@ function transformToRollupOutputChunk(chunk: OutputChunk): RolldownOutputChunk {
}
}

function transformToRollupOutputAsset(asset: OutputAsset): RolldownOutputAsset {
function transformToRollupOutputAsset(asset: BindingOutputAsset): RolldownOutputAsset {
return {
type: 'asset',
fileName: asset.fileName,
source: asset.source,
}
}

export function transformToRollupOutput(output: Outputs): RolldownOutput {
export function transformToRollupOutput(output: BindingOutputs): RolldownOutput {
const { chunks, assets } = output
const [firstChunk, ...restChunks] = chunks
return {
Expand All @@ -38,7 +38,7 @@ export function transformToRollupOutput(output: Outputs): RolldownOutput {
}


export function transformToOutputBundle(output: Outputs): OutputBundle {
export function transformToOutputBundle(output: BindingOutputs): OutputBundle {
return Object.fromEntries(
transformToRollupOutput(output).output.map((item) => [item.fileName, item]),
)
Expand Down

0 comments on commit d094d56

Please sign in to comment.