-
Notifications
You must be signed in to change notification settings - Fork 12.8k
fix(#20846 lib/es2015): override new Proxy<T, U> #44458
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(#20846 lib/es2015): override new Proxy<T, U> #44458
Conversation
@IgorNovozhilov interface B states that proxy instance has member b but it will be undefined since you are not implementing get method in proxy handler imho. Isn't that right? I think that handler implementation should be checked somehow too. |
@dominikdosoudil |
@IgorNovozhilov Yeah, interface A { a: string }
interface B { b: number }
const a: A = { a: '' }
var b = new XProxy<A, B>(a, {})
b.b // undefined because 'get' not provided in handler however your typing says it will be number |
I'm pretty hesitant on this PR, because every time we've introduced extra generics like this into the stdlib a lot of things break unexpectedly. Let's see what happens with the larger test suites. @typescript-bot test this |
@orta Members (and their types) of Proxy depend on its constructor arguments so it cannot have other than generic typing.( However I've shown the bug that brings this PR so I think that there is no point in running tests because if they pass, there is still obvious issue. |
The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need more tests to be able to reason about how that change would work. If you could add the tests I mentioned in comments, that would be a great start. Thanks.
apply?(target: T, thisArg: any, argArray: any[]): any; | ||
construct?(target: T, argArray: any[], newTarget: Function): object; | ||
defineProperty?(target: T, p: string | symbol, attributes: PropertyDescriptor): boolean; | ||
deleteProperty?(target: T, p: string | symbol): boolean; | ||
get?(target: T, p: string | symbol, receiver: any): any; | ||
get?<K extends keyof U>(target: T, p: K, receiver: U): U[K]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you add a test for the program in the original bug? (#20846)
I don't understand how inference of U
would work to fix the bug for that program, since in that case there is no receiver
.
new <T extends object>(target: T, handler: ProxyHandler<T>): T; | ||
revocable<T extends object>(target: T, handler: ProxyHandler<T, any>): { proxy: T; revoke: () => void; }; | ||
revocable<T extends object, U extends object>(target: T, handler: ProxyHandler<T, U>): { proxy: U; revoke: () => void; }; | ||
new <T extends object>(target: T, handler: ProxyHandler<T, any>): T; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't immediately see how this ordering of overloads would work. Could you add tests where TypeScript choses one vs the other overload signatures?
For future reference: if we want to move on with this PR, we should test this with vue and jsdom, which seem to be the only big users of |
@IgorNovozhilov Do you want to keep working on this? Otherwise I'd like to close it to reduce the number of open PRs. |
Fixes #20846
Faced with the need to definition type the Proxy output in my project, I got acquainted with the problem #20846.
I suggest the following solution using overloads, preserving the health of the current behavior and backward compatibility with the current definition format
example:
in VSCode