-
Notifications
You must be signed in to change notification settings - Fork 459
Migrate IntersectionObserverEntry to KDL #2314
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
base: main
Are you sure you want to change the base?
Conversation
|
Thanks for the PR! This section of the codebase is owned by @saschanaz - if they write a comment saying "LGTM" then it will be merged. |
Can you eleborate, like what kind of error? Working around an error is only good when we know what's happening there. |
Scratch that, I have updated the syntax |
| } | ||
| if (signatureObj.param?.length === 0 && !signatureObj.type) { | ||
| // If there are no params and no return type, remove the signature | ||
| signature = undefined; |
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.
This would cause problem for non-removal patches, no?
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.
This wouldn't cause an issue; we already have patches for non-removal methods, and it is working prefectly
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 mean if we want to add a signature... Hmm, but doing so would require a type, so maybe this is fine.
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.
Yes, so this is good. Anything else?
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.
Not "good" but more like "I can live with this for now" for me... but yeah this should be okay.
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.
but constructors don't have a return type, so constructor is also a valid non-removal constructor. Doing this would break adding a parameter-less constructor 🤔
(Meaning we should throw if type exists for constructor btw)
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 will open another one of the throwing an error. It wouldn't count as a removal because the parameters array will be filled. We check for both
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.
Maybe remove it when signatureIndex exists? Having signatureIndex without having params or types don't make sense.
(In that case we'll only remove the constructor/method's signature but not themselves, which I think is fine as it should ultimately behave same)
Also, I added full support for interface removals.
Everything now should be optionally nested
except for the signature, because the emitter throws an error otherwise. So I had to add special handling for it in the converter