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

Fix for the issue #5189 #5209

Merged
merged 1 commit into from
Oct 27, 2015
Merged

Fix for the issue #5189 #5209

merged 1 commit into from
Oct 27, 2015

Conversation

antsmartian
Copy link
Contributor

@jimfb I have written logic for the issue mentioned over here:

#5189

Let me know my thought process is correct; If so I can go and write unit test case for the same.

Thanks for your help.

@@ -213,6 +221,10 @@ var ReactDefaultPerf = {
totalTime,
Array.prototype.slice.call(args, 1)
);

if (fnName === 'putListener') {
ReactDefaultPerf._prevEventListener = typeof isFunction;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm skeptical this would work. I think there is a single ReactDefaultPerf shared for all components; you are setting this field as a static field without regard to the current component.

@jimfb
Copy link
Contributor

jimfb commented Oct 19, 2015

@antoaravinth Looks like there is a failing unit test. Also, we should add a unit test that asserts the intended semantics.

@spicyj is probably the best person to review this PR, since he added the processing of event handlers to printDOM() in the first place.

@sophiebits
Copy link
Collaborator

I would just not instrument putListener/deleteListener at all.

@antsmartian
Copy link
Contributor Author

@jimfb: The moment we are going to ignore the "putListener" events, the test case should not count listener update as waste will be failing, which is expecting the other way around. So probably we need to change our test case and also add additional test cases for our new semantics. What say?

@spicyj Will do the same.

@sophiebits
Copy link
Collaborator

Yes, we can remove that test.

@facebook-github-bot
Copy link

@antoaravinth updated the pull request.

@facebook-github-bot
Copy link

@antoaravinth updated the pull request.

@antsmartian
Copy link
Contributor Author

@jimfb @spicyj Updated. Let me know if everything is fine.

@sophiebits
Copy link
Collaborator

Just remove the instrumentation from EventPluginHub entirely – we don't need it any more.

@sophiebits
Copy link
Collaborator

Sorry for the back and forth.

@sophiebits
Copy link
Collaborator

Can you reference #5189 in your new test case too? It would also be fine to combine them into one test, I think – but two is fine too if you prefer that.

@antsmartian
Copy link
Contributor Author

@spicyj No need to be sorry, thanks for your help on the PR.

Just removing the instrumentation lines :

ReactPerf.measureMethods(EventPluginHub, 'EventPluginHub' , {
   putListener : 'putListener',
   deleteListener : 'deleteListener'
});

from EventPluginHub means we no need to check in our ReactDefaultPerf's measure method whether the passed on fnName is putListener or deleteListener as now they aren't being monitored. Correct me if I'm wrong here.

But at the same time what about the line

else if ( moduleName === 'EventPluginHub' . . 

I guess we can even safely remove that. Because we removed our instrumentation, the above condition is useless there. Throw your thoughts on this.

@sophiebits
Copy link
Collaborator

@antoaravinth Yes, that's right – all that code can go away.

Fix for lint issues

Added the test cases for the issue 5189

Removing empty space

Removing the Eventplugin instrumentation code

Removing unwanted white space
@facebook-github-bot
Copy link

@antoaravinth updated the pull request.

@antsmartian
Copy link
Contributor Author

@spicyj Done. I think the two test cases would be fine but you want me to add the referenced case (#5189) in our test case too? Anyways is fine with me.

sophiebits added a commit that referenced this pull request Oct 27, 2015
Don't instrument listeners for ReactDefaultPerf
@sophiebits sophiebits merged commit 65b9ceb into facebook:master Oct 27, 2015
@sophiebits
Copy link
Collaborator

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants