Skip to content

Comments

refactor(hmr): using esm module namespace to store it's exports#4461

Merged
underfin merged 3 commits intomainfrom
05-09-refactor_hmr_using_esm_module_namespace_to_store_it_s_exports
May 10, 2025
Merged

refactor(hmr): using esm module namespace to store it's exports#4461
underfin merged 3 commits intomainfrom
05-09-refactor_hmr_using_esm_module_namespace_to_store_it_s_exports

Conversation

@underfin
Copy link
Contributor

@underfin underfin commented May 9, 2025

Description

The previous implementation has a problem for cjs.

__rolldown_runtime__.registerModule("cjs.js", module.exports);
exports.a = 1
function b() {
   exports.b = 1
}

The runtime using Object.keys(esmExportGettersOrCjsExports) can't get it's real exports.

The pr fixed it directly using exports at runtime. To compat it with esm, using esm namespace object is suitable.

Copy link
Contributor Author

underfin commented May 9, 2025

@underfin underfin marked this pull request as ready for review May 9, 2025 09:42
@github-actions
Copy link
Contributor

github-actions bot commented May 9, 2025

Benchmarks Rust

  • target: main(9d84ea8)
  • pr: 05-09-refactor_hmr_using_esm_module_namespace_to_store_it_s_exports(136a6df)
group                                                               pr                                     target
-----                                                               --                                     ------
bundle/bundle@multi-duplicated-top-level-symbol                     1.02     83.3±2.73ms        ? ?/sec    1.00     82.0±2.65ms        ? ?/sec
bundle/bundle@multi-duplicated-top-level-symbol-minify              1.00    100.5±1.30ms        ? ?/sec    1.02    102.9±3.72ms        ? ?/sec
bundle/bundle@multi-duplicated-top-level-symbol-minify-sourcemap    1.00    114.4±1.97ms        ? ?/sec    1.03    117.8±2.24ms        ? ?/sec
bundle/bundle@multi-duplicated-top-level-symbol-sourcemap           1.03     92.5±2.25ms        ? ?/sec    1.00     90.1±1.95ms        ? ?/sec
bundle/bundle@rome-ts                                               1.04    127.4±3.46ms        ? ?/sec    1.00    122.8±1.69ms        ? ?/sec
bundle/bundle@rome-ts-minify                                        1.10    224.0±6.68ms        ? ?/sec    1.00    204.1±5.76ms        ? ?/sec
bundle/bundle@rome-ts-minify-sourcemap                              1.07    262.0±6.47ms        ? ?/sec    1.00    243.9±5.47ms        ? ?/sec
bundle/bundle@rome-ts-sourcemap                                     1.02    139.0±2.34ms        ? ?/sec    1.00    135.7±2.70ms        ? ?/sec
bundle/bundle@threejs                                               1.09     45.4±2.59ms        ? ?/sec    1.00     41.8±0.70ms        ? ?/sec
bundle/bundle@threejs-minify                                        1.00     88.5±1.51ms        ? ?/sec    1.00     88.7±1.54ms        ? ?/sec
bundle/bundle@threejs-minify-sourcemap                              1.03    109.2±3.51ms        ? ?/sec    1.00    106.0±3.30ms        ? ?/sec
bundle/bundle@threejs-sourcemap                                     1.00     49.9±0.90ms        ? ?/sec    1.01     50.5±2.07ms        ? ?/sec
bundle/bundle@threejs10x                                            1.01    439.3±9.83ms        ? ?/sec    1.00    435.0±6.57ms        ? ?/sec
bundle/bundle@threejs10x-minify                                     1.01   1073.6±7.19ms        ? ?/sec    1.00   1058.9±4.62ms        ? ?/sec
bundle/bundle@threejs10x-minify-sourcemap                           1.02   1272.8±7.77ms        ? ?/sec    1.00   1246.2±5.50ms        ? ?/sec
bundle/bundle@threejs10x-sourcemap                                  1.00    514.2±4.48ms        ? ?/sec    1.00    514.7±6.40ms        ? ?/sec
remapping/remapping                                                 1.00     26.5±0.78ms        ? ?/sec    1.01     26.9±0.81ms        ? ?/sec
remapping/render-chunk-remapping                                    1.06     67.5±5.86ms        ? ?/sec    1.00     63.5±0.47ms        ? ?/sec
scan/scan@rome-ts                                                   1.00     98.9±2.13ms        ? ?/sec    1.01     99.9±2.39ms        ? ?/sec
scan/scan@threejs                                                   1.00     31.8±1.00ms        ? ?/sec    1.02     32.5±0.62ms        ? ?/sec
scan/scan@threejs10x                                                1.00    318.6±3.45ms        ? ?/sec    1.01    322.6±3.81ms        ? ?/sec

Copy link
Member

@hyf0 hyf0 left a comment

Choose a reason for hiding this comment

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

What about case module.exports = {}?

@underfin underfin force-pushed the 05-08-refactor_hmr_hmr_chunk_loadexports branch from 7100bbd to 251aa8c Compare May 10, 2025 04:22
@underfin
Copy link
Contributor Author

What about case module.exports = {}?

Not sure your meaning.

Base automatically changed from 05-08-refactor_hmr_hmr_chunk_loadexports to main May 10, 2025 05:25
@underfin underfin force-pushed the 05-09-refactor_hmr_using_esm_module_namespace_to_store_it_s_exports branch from dce4417 to 5d53810 Compare May 10, 2025 06:19
@underfin underfin enabled auto-merge May 10, 2025 06:19
@netlify
Copy link

netlify bot commented May 10, 2025

Deploy Preview for rolldown-rs canceled.

Name Link
🔨 Latest commit 136a6df
🔍 Latest deploy log https://app.netlify.com/sites/rolldown-rs/deploys/681f4f93b6471400082a9b8c

@hyf0
Copy link
Member

hyf0 commented May 10, 2025

What about case

__rolldown_runtime__.registerModule("cjs.js", module.exports);
module.exports = {
  a: 1,
  b: 1,
}

@underfin underfin force-pushed the 05-09-refactor_hmr_using_esm_module_namespace_to_store_it_s_exports branch from 6c65ff0 to 136a6df Compare May 10, 2025 13:07
@underfin underfin added this pull request to the merge queue May 10, 2025
Merged via the queue into main with commit 05b3e10 May 10, 2025
30 checks passed
@underfin underfin deleted the 05-09-refactor_hmr_using_esm_module_namespace_to_store_it_s_exports branch May 10, 2025 13:32
github-merge-queue bot pushed a commit that referenced this pull request Jun 27, 2025
`__rolldown_runtime__.__toCommonJS(namespace);` was generated in the HMR
code. But this call is no-op (the return value is not used).

```js
var __toCommonJS = mod => __copyProps(__defProp({}, '__esModule', { value: true }), mod)
var __copyProps = (to, from, except, desc) => {
  if (from && typeof from === 'object' || typeof from === 'function')
    for (var keys = __getOwnPropNames(from), i = 0, n = keys.length, key; i < n; i++) {
      key = keys[i]
      if (!__hasOwnProp.call(to, key) && key !== except)
        __defProp(to, key, { get: (k => from[k]).bind(null, key), enumerable: !(desc = __getOwnPropDesc(from, key)) || desc.enumerable })
    }
  return to
}
var __defProp = Object.defineProperty
```

Also this call is slow because it copies all properties.

This seems to be added by #4461
but I'm not sure why this was added.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants