Skip to content

fix: JSON.parse() should not have side effect#2029

Merged
IWANABETHATGUY merged 2 commits intomainfrom
fix/json-parse-should-not-have-side-effects
Aug 21, 2024
Merged

fix: JSON.parse() should not have side effect#2029
IWANABETHATGUY merged 2 commits intomainfrom
fix/json-parse-should-not-have-side-effects

Conversation

@IWANABETHATGUY
Copy link
Member

@IWANABETHATGUY IWANABETHATGUY commented Aug 20, 2024 •
edited
Loading

Description

  1. Before the test case will fail, since JSON.parse is considered to have side effects.

IWANABETHATGUY
IWANABETHATGUY commented Aug 20, 2024
View reviewed changes
packages/rolldown/tests/fixtures/builtin-plugin/json/side-effects/main.js
@@ -0,0 +1,3 @@
import json from './package.json'
Copy link
Member Author

@IWANABETHATGUY IWANABETHATGUY Aug 20, 2024

Choose a reason for hiding this comment

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

Make json unused.

@netlify
Copy link

netlify bot commented Aug 20, 2024 •
edited
Loading

✅ Deploy Preview for rolldown-rs canceled.

Name Link
🔨 Latest commit 9b29935
🔍 Latest deploy log https://app.netlify.com/sites/rolldown-rs/deploys/66c4dab6b3b2540008e7857d

@github-actions
Copy link
Contributor

github-actions bot commented Aug 20, 2024

Benchmarks Rust

  • target: main(660c4bd)
  • pr: fix/json-parse-should-not-have-side-effects(9b29935)
group                                                               pr                                     target
-----                                                               --                                     ------
bundle/bundle@multi-duplicated-top-level-symbol                     1.00     55.3±1.12ms        ? ?/sec    1.01     56.0±1.48ms        ? ?/sec
bundle/bundle@multi-duplicated-top-level-symbol-minify              1.00     77.9±1.16ms        ? ?/sec    1.04     80.9±2.81ms        ? ?/sec
bundle/bundle@multi-duplicated-top-level-symbol-minify-sourcemap    1.01     98.4±1.91ms        ? ?/sec    1.00     97.3±1.77ms        ? ?/sec
bundle/bundle@multi-duplicated-top-level-symbol-sourcemap           1.00     62.9±0.98ms        ? ?/sec    1.02     64.2±0.93ms        ? ?/sec
bundle/bundle@rome-ts                                               1.00    103.6±1.16ms        ? ?/sec    1.00    103.8±1.19ms        ? ?/sec
bundle/bundle@rome-ts-minify                                        1.00    192.2±5.36ms        ? ?/sec    1.04    199.7±5.65ms        ? ?/sec
bundle/bundle@rome-ts-minify-sourcemap                              1.00    236.3±6.12ms        ? ?/sec    1.03    243.3±6.82ms        ? ?/sec
bundle/bundle@rome-ts-sourcemap                                     1.00    119.9±1.85ms        ? ?/sec    1.00    120.0±1.33ms        ? ?/sec
bundle/bundle@threejs                                               1.00     37.4±0.40ms        ? ?/sec    1.01     37.7±0.23ms        ? ?/sec
bundle/bundle@threejs-minify                                        1.00     84.5±2.74ms        ? ?/sec    1.03     86.6±1.92ms        ? ?/sec
bundle/bundle@threejs-minify-sourcemap                              1.00    101.5±2.24ms        ? ?/sec    1.03    104.1±2.42ms        ? ?/sec
bundle/bundle@threejs-sourcemap                                     1.00     46.9±0.74ms        ? ?/sec    1.02     47.7±0.73ms        ? ?/sec
bundle/bundle@threejs10x                                            1.00    396.6±3.52ms        ? ?/sec    1.00    395.9±3.35ms        ? ?/sec
bundle/bundle@threejs10x-minify                                     1.01   1022.8±7.47ms        ? ?/sec    1.00  1017.5±17.73ms        ? ?/sec
bundle/bundle@threejs10x-minify-sourcemap                           1.00  1253.0±13.65ms        ? ?/sec    1.00  1253.0±12.02ms        ? ?/sec
bundle/bundle@threejs10x-sourcemap                                  1.00    463.4±5.08ms        ? ?/sec    1.00    464.6±5.13ms        ? ?/sec
remapping/remapping                                                 1.00     32.7±0.32ms        ? ?/sec    1.00     32.8±0.12ms        ? ?/sec
remapping/render-chunk-remapping                                    1.00     81.4±0.52ms        ? ?/sec    1.02     82.8±0.40ms        ? ?/sec
scan/scan@rome-ts                                                   1.00     83.6±1.27ms        ? ?/sec    1.00     84.1±0.89ms        ? ?/sec
scan/scan@threejs                                                   1.00     29.8±0.24ms        ? ?/sec    1.00     29.9±0.36ms        ? ?/sec
scan/scan@threejs10x                                                1.00    297.0±2.76ms        ? ?/sec    1.00    297.3±2.72ms        ? ?/sec

@IWANABETHATGUY IWANABETHATGUY marked this pull request as ready for review August 21, 2024 03:24
@IWANABETHATGUY IWANABETHATGUY added this pull request to the merge queue Aug 21, 2024
Merged via the queue into main with commit 4ca169c Aug 21, 2024
@IWANABETHATGUY IWANABETHATGUY deleted the fix/json-parse-should-not-have-side-effects branch August 21, 2024 03:34
@hi-ogawa hi-ogawa mentioned this pull request Jan 13, 2025
Merged
github-merge-queue bot pushed a commit that referenced this pull request Jan 13, 2025
@hi-ogawa
fix(plugin/json): tree shake json named exports (#3346)
b8f0e19 
<!-- Thank you for contributing! -->

### Description

This was once fixed in #2029,
but #3103 added another
`JSON.parse` without pure comment.

Someone noticed the same bug on Vite
vitejs/vite#19189, so I ported to here.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@underfin underfin underfin approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

needs-triage

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@IWANABETHATGUY @underfin