Skip to content

Comments

test(dev): migrate hmr tests using new dev API#5980

Closed
hyf0 wants to merge 1 commit intographite-base/5980from
08-30-test_dev_migrate_hmr_tests_using_new_dev_api
Closed

test(dev): migrate hmr tests using new dev API#5980
hyf0 wants to merge 1 commit intographite-base/5980from
08-30-test_dev_migrate_hmr_tests_using_new_dev_api

Conversation

@hyf0
Copy link
Member

@hyf0 hyf0 commented Aug 30, 2025

Related code is merged into #5979.

Copy link
Member Author

hyf0 commented Aug 30, 2025

@hyf0 hyf0 force-pushed the 08-30-feat_dev_support_import.meta.invalidate_ branch from 6ebbbdc to 8a22b7d Compare August 30, 2025 14:05
@hyf0 hyf0 force-pushed the 08-30-test_dev_migrate_hmr_tests_using_new_dev_api branch from 35ebf7f to ea4173c Compare August 30, 2025 14:05
if (this.hasLiveConnections) {
const output = await build.hmrInvalidate(msg.moduleId);
if (this.#sockets.size > 0) {
const output = await this.#devEngine!.invalidate(msg.moduleId);
Copy link
Contributor

Choose a reason for hiding this comment

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

The non-null assertion operator (!) on this.#devEngine could lead to a runtime error if #handleHmrInvalidate is called before #devEngine is initialized in the serve() method. Consider adding a null check before using the engine or ensuring proper initialization sequence. A safer approach would be:

if (!this.#devEngine) {
  console.error('Dev engine not initialized');
  return;
}
const output = await this.#devEngine.invalidate(msg.moduleId);

This prevents potential null pointer dereferences while making the code more robust against timing issues.

Suggested change
const output = await this.#devEngine!.invalidate(msg.moduleId);
if (!this.#devEngine) {
console.error('Dev engine not initialized');
return;
}
const output = await this.#devEngine.invalidate(msg.moduleId);

Spotted by Diamond

Fix in Graphite


Is this helpful? React 👍 or 👎 to let us know.

@hyf0 hyf0 changed the base branch from 08-30-feat_dev_support_import.meta.invalidate_ to graphite-base/5980 August 30, 2025 14:09
@hyf0 hyf0 closed this Aug 30, 2025
@github-actions
Copy link
Contributor

Benchmarks Rust

  • target: 08-30-feat_dev_support_import.meta.invalidate_(8a22b7d)
  • pr: 08-30-test_dev_migrate_hmr_tests_using_new_dev_api(ea4173c)
group                                                        pr                                     target
-----                                                        --                                     ------
bundle/bundle@multi-duplicated-top-level-symbol              1.05     82.2±2.25ms        ? ?/sec    1.00     78.4±1.52ms        ? ?/sec
bundle/bundle@multi-duplicated-top-level-symbol-sourcemap    1.03     92.1±1.73ms        ? ?/sec    1.00     89.1±2.05ms        ? ?/sec
bundle/bundle@rome_ts                                        1.01    119.4±2.92ms        ? ?/sec    1.00    117.8±1.37ms        ? ?/sec
bundle/bundle@rome_ts-sourcemap                              1.00    139.5±2.50ms        ? ?/sec    1.00    139.2±2.13ms        ? ?/sec
bundle/bundle@threejs                                        1.01     44.3±1.44ms        ? ?/sec    1.00     44.0±2.73ms        ? ?/sec
bundle/bundle@threejs-sourcemap                              1.02     53.5±2.75ms        ? ?/sec    1.00     52.6±1.89ms        ? ?/sec
bundle/bundle@threejs10x                                     1.00    463.5±4.33ms        ? ?/sec    1.00    463.4±6.10ms        ? ?/sec
bundle/bundle@threejs10x-sourcemap                           1.00    545.1±5.13ms        ? ?/sec    1.00    546.8±6.20ms        ? ?/sec
scan/scan@rome_ts                                            1.00     93.4±2.46ms        ? ?/sec    1.00     93.4±1.35ms        ? ?/sec
scan/scan@threejs                                            1.00     33.2±1.96ms        ? ?/sec    1.02     33.8±2.47ms        ? ?/sec
scan/scan@threejs10x                                         1.00    346.9±5.16ms        ? ?/sec    1.01    350.7±6.01ms        ? ?/sec

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.

1 participant