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.

Sign up for GitHub

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

Jump to bottom

refactor(reactivity): priority check if the object is invalid #10419

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

refactor(reactivity): priority check if the object is invalid #10419

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
656135b
refactor(reactivity): priority check if the object is invalid
OnlyWick Feb 27, 2024
3fee4e2
Merge branch 'main' into priority-check-invalid
OnlyWick Feb 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
15 changes: 15 additions & 0 deletions packages/reactivity/__tests__/reactive.spec.ts
View file
Open in desktop
Expand Up @@ -302,4 +302,19 @@ describe('reactivity/reactive', () => {
const observed = reactive(original)
expect(isReactive(observed)).toBe(false)
})

test('should not check cache', () => {
const foo = {}
Object.freeze(foo)
const noObserved = reactive(foo)
expect(noObserved).toBe(foo)

const bar = {}
// put in cache
reactive(bar)
Object.freeze(bar)
// do not check cache
const observed = reactive(bar)
expect(observed).toBe(bar)
})
})
16 changes: 4 additions & 12 deletions packages/reactivity/src/reactive.ts
View file
Open in desktop
Expand Up @@ -252,24 +252,16 @@ function createReactiveObject(
}
return target
}
// target is already a Proxy, return it.
// exception: calling readonly() on a reactive object
if (
target[ReactiveFlags.RAW] &&
!(isReadonly && target[ReactiveFlags.IS_REACTIVE])
) {
const targetType = getTargetType(target)
Copy link
Member

Choose a reason for hiding this comment

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

https://deploy-preview-10419--vue-sfc-playground.netlify.app/#eNp9UstOwzAQ/JWVLwWpJGrhFKWVAPUAB4qAoy95bFOXxLZsJ1RE+XfWrtIWhHrzzsyOZqzt2b3WUdciS1hqCyO0A4uu1UsuRaOVcdCDwaxwokMYYGNUAxPST7jkslDSOmhsBQvos6TPE5jMJsPAZRyDbh0ICUVWbPFMOiftaHhF8zWX63yHhYs2BvE7YHMCyaFUIJUD2i8+f/uo3KLpsPzj5ffS+FCDCtDgsNF15pAmAEi3s2Xfn7aHIY0J8lwqpA/c3TSqxHrB2SiKsijnDGJSpfGZHZsyZynNRlTRzipJH9h7I84K1WhRo1lrJygtZwkExnNZXauv54A50+J0xEPHf/Cd3XuMs1eDIQ9nR85lpkJ3oFfvL7in95GkGm1N6gvkG1pVtz7jQfbQypJin+lC2qdwBkJWH3a1dyjtWMoH9coh6Dmjq3i8UP0U9za6C3tcDmz4Ab7Q11g=

In the playground, using Object.freeze to handle proxy objects will result in an error. There should be very few people who would freeze a proxy object.

Copy link
Contributor Author

@OnlyWick OnlyWick Feb 28, 2024 •
edited

Choose a reason for hiding this comment

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

Yeah, Object.freeze() is rarely used. I believe that instead of checking the cache first, we should ensure that the object is not INVALID before considering the subsequent actions.

https://deploy-preview-10419--vue-sfc-playground.netlify.app/#eNp9UkFOwzAQ/MrKl4JUEhU4VWklQD3AgSLg6EvqbFOXxLbsTVoR5e9sUqUtAvXmnZkdzVjbiAfnorpCMRVJUF47goBUubk0unTWEzTgMVWkaxzDLiW1gRbW3pYw4rWRNNIoawJBGXKYQZNOm9UURpNR20oTx+AqAm1ApWqDZ9Jb1g6+VzxfS7NcbVFRtPaI3wPGBpkFYwl4XX39trGrgL7G7K9VEh+6cAseCEtXpIQ8AUCymcyb5rTctknMUMcl2nRx65vSZljMpBhEURqtpICYVUl8ZifGggKHWes82gZr+BebzkgKZUunC/RLR5rDSjGFnum4tCjs7qXHyFc4HvC+4j/4Nuw7TIo3j30eKY4cpT5HOtCLj1fc8/tIco2qYPUF8h2DLaou40H2WJmMY5/p+rTP/S1ok3+GxZ7QhKFUF7RTtr1eCr6JpwvVT3Hvovt+T5pWtD+UnNlX

Although this example is peculiar, in case there is a large amount of code within a single SFC, maintained by multiple people, if someone misuses Object.freeze() and another person proxies the original object, an error would be thrown in the now Vue version. However, if the object is INVALID, just return the original object directly, in your case, the error won't occur.

const isBaseOnReactiveTarget = isReadonly && target[ReactiveFlags.IS_REACTIVE]
const isInvalid = targetType === TargetType.INVALID
if ((target[ReactiveFlags.RAW] && !isBaseOnReactiveTarget) || isInvalid) {
return target
}
// target already has corresponding Proxy
const existingProxy = proxyMap.get(target)
if (existingProxy) {
return existingProxy
}
// only specific value types can be observed.
const targetType = getTargetType(target)
if (targetType === TargetType.INVALID) {
return target
}
const proxy = new Proxy(
target,
targetType === TargetType.COLLECTION ? collectionHandlers : baseHandlers,
Expand Down