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

Use cached noop function #33496

Merged
merged 5 commits into from Apr 11, 2021
Merged

Use cached noop function #33496

merged 5 commits into from Apr 11, 2021

Conversation

rohit2sharma95
Copy link
Collaborator

noop function returns a new function every time whenever it is called. So just use the noop function, not the value returned by it (So that the event handler can remove the same function that was attached before).

Ref: #33451 (comment)

@rohit2sharma95 rohit2sharma95 requested a review from a team as a code owner March 27, 2021 17:28
@rohit2sharma95 rohit2sharma95 added this to Inbox in v5.0.0 via automation Mar 27, 2021
@rohit2sharma95 rohit2sharma95 moved this from Inbox to Review in v5.0.0 Mar 27, 2021
@XhmikosR
Copy link
Member

I recall noticing this in the past but forgot to make a separate PR for sure. :) That being said, shouldn't our tests have caught this? I mean null !== noop(). The extra args are OK, though.

@XhmikosR XhmikosR requested a review from GeoSot March 30, 2021 12:50
@GeoSot
Copy link
Member

GeoSot commented Mar 30, 2021

@XhmikosR I suppose, you remember this
Do we still need this tweak @patrickhlauke ?

@XhmikosR
Copy link
Member

XhmikosR commented Apr 5, 2021

Yeah, that's another change :) But I still wonder why this wasn't caught :/

@rohit2sharma95
Copy link
Collaborator Author

Do we still need this tweak @patrickhlauke?

Not sure about that but that should be considered in another PR IMO (If that is not needed) @GeoSot. 🙂
So this PR is complete and will help #33466 to proceed.

v5.0.0 automation moved this from Review to Approved Apr 9, 2021
@XhmikosR XhmikosR merged commit bac0b0c into main Apr 11, 2021
v5.0.0 automation moved this from Approved to Done Apr 11, 2021
@XhmikosR XhmikosR deleted the rohit/noop-handling branch April 11, 2021 06:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
v5.0.0
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants