Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: unify js dependency #8394

Merged
merged 12 commits into from
Nov 13, 2024
Merged

feat: unify js dependency #8394

merged 12 commits into from
Nov 13, 2024

Conversation

SyMind
Copy link
Member

@SyMind SyMind commented Nov 11, 2024

Summary

feat: unify js dependency

Checklist

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

@github-actions github-actions bot added the release: feature release: feature related release(mr only) label Nov 11, 2024
Copy link

netlify bot commented Nov 11, 2024

Deploy Preview for rspack canceled.

Name Link
🔨 Latest commit 4162479
🔍 Latest deploy log https://app.netlify.com/sites/rspack/deploys/67342532003f4600086d3b89

@SyMind
Copy link
Member Author

SyMind commented Nov 11, 2024

!bench

@SyMind
Copy link
Member Author

SyMind commented Nov 11, 2024

!eco-ci

@rspack-bot
Copy link

rspack-bot commented Nov 11, 2024

📝 Benchmark detail: Open

Name Base (2024-11-11 2ac4a11) Current Change
10000_big_production-mode + exec 44.3 s ± 799 ms 44.4 s ± 1.13 s +0.27 %
10000_development-mode + exec 1.85 s ± 27 ms 1.84 s ± 19 ms -0.33 %
10000_development-mode_hmr + exec 642 ms ± 6.8 ms 644 ms ± 8.2 ms +0.31 %
10000_production-mode + exec 2.44 s ± 34 ms 2.45 s ± 66 ms +0.45 %
arco-pro_development-mode + exec 1.78 s ± 79 ms 1.79 s ± 61 ms +0.87 %
arco-pro_development-mode_hmr + exec 430 ms ± 0.53 ms 430 ms ± 1.3 ms -0.05 %
arco-pro_production-mode + exec 3.17 s ± 72 ms 3.2 s ± 59 ms +0.88 %
arco-pro_production-mode_generate-package-json-webpack-plugin + exec 3.24 s ± 79 ms 3.25 s ± 94 ms +0.24 %
threejs_development-mode_10x + exec 1.58 s ± 13 ms 1.59 s ± 16 ms +0.15 %
threejs_development-mode_10x_hmr + exec 774 ms ± 12 ms 779 ms ± 20 ms +0.70 %
threejs_production-mode_10x + exec 4.96 s ± 35 ms 4.98 s ± 12 ms +0.27 %

@rspack-bot
Copy link

rspack-bot commented Nov 11, 2024

📝 Ran ecosystem CI: Open

suite result
modernjs ✅ success
rspress ✅ success
rslib ✅ success
rsbuild ✅ success
examples ✅ success
devserver ✅ success

@SyMind SyMind force-pushed the unify-js-dependency branch from eb6991a to 820826c Compare November 11, 2024 07:22
@SyMind SyMind marked this pull request as ready for review November 11, 2024 07:50
@SyMind SyMind requested a review from jerrykingxyz as a code owner November 11, 2024 07:50
@JSerFeng
Copy link
Contributor

This pr depends on accessing a field within a raw pointer, for example

// ptr is raw pointer and might point to whatever possible
let foo = ptr.as_ref();

foo.id == last_id

But I wonder if access id here will panic ? And this is a solution that relies on UB ? Can we accept this @h-a-n-a

@SyMind SyMind force-pushed the unify-js-dependency branch from 8acb83d to 5b2e260 Compare November 11, 2024 13:01
@h-a-n-a
Copy link
Contributor

h-a-n-a commented Nov 12, 2024

But I wonder if access id here will panic ? And this is a solution that relies on UB ? Can we accept this @h-a-n-a

No, this will not panic and it's more likely a undefined behavior if the memory you're accessing to is freed and it will likely to cause segmentation faults. All we need to do is to prevent user from accessing that value after the scope is dropped.

@SyMind SyMind force-pushed the unify-js-dependency branch 2 times, most recently from 67e7e90 to d9844db Compare November 13, 2024 03:07
@SyMind SyMind force-pushed the unify-js-dependency branch from d9844db to 4162479 Compare November 13, 2024 04:04
@SyMind SyMind merged commit 78aa2e0 into main Nov 13, 2024
29 of 30 checks passed
@SyMind SyMind deleted the unify-js-dependency branch November 13, 2024 08:26
@h-a-n-a h-a-n-a mentioned this pull request Nov 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release: feature release: feature related release(mr only)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants