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

refactor(reactivity): don't convert Set into array #8718

Closed
wants to merge 5 commits into from

Conversation

Alfred-Skyblue
Copy link
Member

close #8714

@sxzz
Copy link
Member

sxzz commented Jul 19, 2023

/ecosystem-ci run

@vue-bot
Copy link
Contributor

vue-bot commented Jul 19, 2023

📝 Ran ecosystem CI: Open

suite result
nuxt ✅ success
pinia ✅ success
quasar ❌ failure
router ✅ success
test-utils ✅ success
vant ✅ success
vite-plugin-vue ✅ success
vitepress ❌ failure
vue-i18n ✅ success
vue-macros ❌ failure
vuetify ✅ success
vueuse ✅ success
vue-simple-compiler ✅ success

@sxzz sxzz changed the title chore: there is no need to convert the Set into an array refactor(reactivity): don't convert Set into array Aug 15, 2023
@skirtles-code
Copy link
Contributor

Note for reviewers: I think there's some overlap with other PRs: #6185, #7827 and #8947.

Comment on lines -397 to -398
// spread into array for stabilization
const effects = isArray(dep) ? dep : [...dep]
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe the reason for copying the dep into an array is to ensure that the subsequent iterations are stable. The calls to triggerEffect() inside those loops could modify dep while we're trying to iterate over it.

The PR description and linked issue don't seem to mention this, so I'm wondering whether there's been a misunderstanding about the purpose of the array?

@johnsoncodehk
Copy link
Member

Also resolved by #5912. Note that dep could change during iterations, but in #5912 it will only change after iterations, so the spread can be safely removed.

@Alfred-Skyblue Alfred-Skyblue deleted the chore-dep branch October 19, 2023 08:50
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.

In triggerEffects, it is not necessary to determine whether the dep parameter is an Array
5 participants