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

fix(runtime-core): correctly assign suspenseId to avoid conflicts with the default id #9966

Merged
merged 6 commits into from Jan 3, 2024

Conversation

yangxiuxiu1115
Copy link
Contributor

fix #9944

Copy link

github-actions bot commented Jan 1, 2024

Size Report

Bundles

File Size Gzip Brotli
runtime-dom.global.prod.js 89.5 kB (+3 B) 34 kB (+2 B) 30.8 kB (+51 B)
vue.global.prod.js 146 kB (+3 B) 53.3 kB (+3 B) 47.7 kB (+39 B)

Usages

Name Size Gzip Brotli
createApp 49.8 kB 19.5 kB 17.8 kB
createSSRApp 53.1 kB 20.8 kB 19 kB
defineCustomElement 52.1 kB 20.3 kB 18.5 kB
overall 63.3 kB (+3 B) 24.5 kB (+3 B) 22.2 kB (-52 B)

@pikax pikax added the need test The PR has missing test cases. label Jan 2, 2024
@edison1105
Copy link
Member

edison1105 commented Jan 2, 2024


Or we can change the initial value to pendingId: suspenseId++

@yangxiuxiu1115
Copy link
Contributor Author

I am not very familiar with e2e testing, this is playground . I cannot accurately reproduce in the test file, I need some help.

@edison1105
Copy link
Member

edison1105 commented Jan 3, 2024

@RicardoErii
You don't need to add e2e tests. Some test cases in Suspense.spec.ts are likely problematic. The current test cases pass because suspenseId does not start from 0. I don't have permission to modify your PR directly, so I'm providing the solution here:

  • Add export const resetSuspenseId = () => suspenseId = 0 in Suspense.ts to reset suspenseId.
  • Call resetSuspenseId() in the beforeEach of Suspense.spec.ts to ensure that suspenseId starts from 0 for each test case.
  • Revert the changes in your PR.
  • run test nr test Suspense.spec.ts. At this point, you will notice that some test cases in Suspense.spec.ts will fail.

Apply the changes in your PR and all test cases will pass

@yangxiuxiu1115
Copy link
Contributor Author

NB, Thank you, then I won't add test cases

@edison1105
Copy link
Member

@RicardoErii
No additional test cases need to be added, but the relevant logic for resetSuspenseId needs to be added. To make sure there is no regression in the future.

@yyx990803 yyx990803 merged commit 0648804 into vuejs:main Jan 3, 2024
11 checks passed
@yangxiuxiu1115 yangxiuxiu1115 deleted the fix/suspence branch January 6, 2024 16:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need test The PR has missing test cases.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Suspense regression of update and replace a async component at same time in version 3.4.0
4 participants