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: the reactive set supports the difference, intersection, isDisjointFrom, isSubsetOf, isSupersetOf, and symmetricDifference methods #11399

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

linzhe141
Copy link
Contributor

@linzhe141 linzhe141 commented Jul 19, 2024

close #11398

It requires Node v22

Copy link

github-actions bot commented Jul 19, 2024

Size Report

Bundles

File Size Gzip Brotli
runtime-dom.global.prod.js 91.2 kB (+1.22 kB) 34.7 kB (+254 B) 31.3 kB (+212 B)
vue.global.prod.js 148 kB (+1.22 kB) 54.2 kB (+251 B) 48.2 kB (+196 B)

Usages

Name Size Gzip Brotli
createApp 50.8 kB (+1.02 kB) 19.7 kB (+143 B) 18 kB (+136 B)
createSSRApp 54.3 kB (+1.02 kB) 21.1 kB (+145 B) 19.3 kB (+133 B)
defineCustomElement 53 kB (+1.02 kB) 20.4 kB (+160 B) 18.6 kB (+136 B)
overall 64.3 kB (+1.02 kB) 24.7 kB (+141 B) 22.4 kB (+121 B)

@sxzz
Copy link
Member

sxzz commented Jul 19, 2024

Could you please add a unit test?

@linzhe141
Copy link
Contributor Author

linzhe141 commented Jul 19, 2024

Could you please add a unit test?

这里很多的api,好像都不支持

image

好像node22才能支持这些新增的api,所以单测没过

…tFrom, isSubsetOf, isSupersetOf, and symmetricDifference` methods
@linzhe141 linzhe141 changed the title feat(reactivity): set supurts intersection feat: the reactive set supports the difference, intersection, isDisjointFrom, isSubsetOf, isSupersetOf, and symmetricDifference methods Jul 19, 2024
@@ -173,6 +173,24 @@ function createForEach(isReadonly: boolean, isShallow: boolean) {
}
}

function createSetProtoMethod(method: TriggerOpTypes) {
return function (this: SetTypes, value: unknown, _isShallow = false) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is anything passing the _isShallow flag? Is it actually needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is really not necessary

if (!hadKey) {
// @ts-expect-error
result = target[method](value)
trigger(target, method, value, value)
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be tracking rather than triggering?

These methods don't currently track correctly inside reactive effects. e.g.:

The unit tests aren't really testing this, as the effect isn't wrapped around the method call.

I haven't confirmed this, but I suspect these methods could reuse TrackOpTypes.ITERATE.

}
const target = toRaw(this)
const proto = getProto(target)
const hadKey = proto.has.call(target, value)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand what this is for. The name value here is a bit misleading, as it refers to the other set. Why do we need to check whether one set is inside the other? This seems to break the methods if one set contains the other:

@linzhe141 linzhe141 marked this pull request as draft July 22, 2024 01:11
@linzhe141 linzhe141 marked this pull request as ready for review July 22, 2024 11:55
@edison1105 edison1105 added ✨ feature request New feature or request version: minor 🛑 on hold This PR can't be merged until other work is finished ready for review This PR requires more reviews labels Aug 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🛑 on hold This PR can't be merged until other work is finished ready for review This PR requires more reviews ✨ feature request New feature or request version: minor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cannot call Set#intersection for set with Proxy
4 participants