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

Add warning when reading from event which has been returned to the pool #5940

Merged
merged 1 commit into from Feb 18, 2016

Conversation

@kentcdodds
Copy link
Contributor

@kentcdodds kentcdodds commented Jan 29, 2016

This is a WIP. I just want to make sure that I'm headed in the right direction for solving #5939

I'm not certain where the logic for deconstructing SyntheticEvents occurs. My guess is it's an abstraction that utilizes the EventInterface.

Also, what's the proper way to reference NODE_ENV for doing this only in development mode.

Thank you for helping a newbie to the codebase :-)

@gaearon
Copy link
Member

@gaearon gaearon commented Jan 29, 2016

Rather than throw, I think it should generate a warning.
Here is a another work-in-progress PR you can use as a reference: #5744

@gaearon
Copy link
Member

@gaearon gaearon commented Jan 29, 2016

I think these lines might be relevant:

for (var propName in Interface) {
this[propName] = null;
}

@kentcdodds
Copy link
Contributor Author

@kentcdodds kentcdodds commented Jan 29, 2016

Rather than throw, I think it should generate a warning.

Ah, yes, that's right. Thanks for the reference 👍

@kentcdodds kentcdodds force-pushed the kentcdodds:pr/warn-event-pool-access branch from cc49323 to 024c001 Jan 29, 2016
@facebook-github-bot
Copy link

@facebook-github-bot facebook-github-bot commented Jan 29, 2016

@kentcdodds updated the pull request.

expect(console.error.calls.length).toBe(1);
expect(console.error.argsForCall[0][0]).toBe(
'Warning: This synthetic event is reused for performance reasons. If ' +
'you\'re seeing this, you\'re accessing a propertie on a ' +

This comment has been minimized.

@zpao

zpao Jan 29, 2016
Member

"property" :)

@zpao
Copy link
Member

@zpao zpao commented Jan 29, 2016

https://github.com/facebook/react/blob/master/src/shared/utils/PooledClass.js#L102-L111 is another place to look. That's what get's run to add pooling to a class, generating a new class with a static release method which calls the destructor (as @gaearon linked to).

@gaearon
Copy link
Member

@gaearon gaearon commented Jan 29, 2016

Another thing is you might want to put the warning code into a “devtool” which is a new work-in-progress API for doing dev-only things. See 251d6c3 and #5590 for inspiration.

@zpao
Copy link
Member

@zpao zpao commented Jan 29, 2016

Might be tricky as a "devtool" since you need to add getters, which doesn't fit so well into the devtool event framework (at least as I understand it). Definitely work looking into though

@kentcdodds kentcdodds force-pushed the kentcdodds:pr/warn-event-pool-access branch from 024c001 to e808985 Jan 29, 2016
@kentcdodds
Copy link
Contributor Author

@kentcdodds kentcdodds commented Jan 29, 2016

Updated. This is technically working, but there are some potential issues that I'll add some inline comments about.

it('should warn if the synthetic event has been released when calling `preventDefault`', function() {
spyOn(console, 'error');
var syntheticEvent = createEvent({});
SyntheticEvent.release(syntheticEvent);
syntheticEvent.preventDefault();
expect(console.error.calls.length).toBe(1);
expect(console.error.argsForCall[0][0]).toBe(
expect(console.error.calls.length).toBe(3); // once each for setting `defaultPrevented`, accessing `nativeEvent`, and accessing `preventDefault`

This comment has been minimized.

@kentcdodds

kentcdodds Jan 29, 2016
Author Contributor

When calling preventDefault, we set the defaultPrevented access the nativeEvent properties. This leads to three warnings even if the developer only called preventDefault.

spyOn(console, 'error');
var syntheticEvent = createEvent({});
SyntheticEvent.release(syntheticEvent);
syntheticEvent.stopPropagation();
expect(console.error.calls.length).toBe(1);
expect(console.error.argsForCall[0][0]).toBe(
expect(console.error.calls.length).toBe(2); // once each for accessing `nativeEvent` and accessing `setPropogation`

This comment has been minimized.

@kentcdodds

kentcdodds Jan 29, 2016
Author Contributor

Similar situation as mentioned above

@@ -95,13 +126,13 @@ describe('SyntheticEvent', function() {
);
});

it('should warn if the synthetic event has been released when calling `stopPropagation`', function() {
iit('should warn if the synthetic event has been released when calling `stopPropagation`', function() {

This comment has been minimized.

@kentcdodds

kentcdodds Jan 29, 2016
Author Contributor

whoops :-) fixing now

@kentcdodds
Copy link
Contributor Author

@kentcdodds kentcdodds commented Jan 29, 2016

On possible suggestion is to set a property on the event called _nullified or _released and check for that before trying to access any properties.

@facebook-github-bot
Copy link

@facebook-github-bot facebook-github-bot commented Jan 29, 2016

@kentcdodds updated the pull request.

@kentcdodds
Copy link
Contributor Author

@kentcdodds kentcdodds commented Jan 29, 2016

Heh... Still got some work on this, I've got quite a few failing tests in the full test suite and some odd behavior in the stopPropogation test (looks like console.error is called 14 times with my warning for some reason).

@kentcdodds
Copy link
Contributor Author

@kentcdodds kentcdodds commented Jan 29, 2016

I think the problem is that when we restore an event, we need to re-defineProperty the object (only in __DEV__) otherwise it will run through my getter/setter and log the warning.

Let me know if that sounds wrong. I'll push what I've got so far for review and keep working on it

@kentcdodds kentcdodds force-pushed the kentcdodds:pr/warn-event-pool-access branch from e808985 to 2c688f2 Jan 29, 2016
@facebook-github-bot
Copy link

@facebook-github-bot facebook-github-bot commented Jan 29, 2016

@kentcdodds updated the pull request.

@kentcdodds kentcdodds force-pushed the kentcdodds:pr/warn-event-pool-access branch from 2c688f2 to fa5582e Jan 29, 2016
@kentcdodds
Copy link
Contributor Author

@kentcdodds kentcdodds commented Jan 29, 2016

Great. I'm ready for feedback now. All tests are passing. I have a linting question I'll add as an inline comment. I'm not solid on this approach, so definitely willing to make changes to how things work or the style of the code. 👍

warning(
warningCondition,
'This synthetic event is reused for performance reasons. If you\'re ' +
'seeing this, you\'re setting property `' + propName + '` on a ' +

This comment has been minimized.

@kentcdodds

kentcdodds Jan 29, 2016
Author Contributor

I'm getting a linting error:

190:13  error  The second argument to warning must be a string literal  react-internal/warning-and-invariant-args

I think it's because of this line. Is there a reason I can't provide the propName here? I feel like it would be useful to have it.

This comment has been minimized.

@gaearon

gaearon Jan 29, 2016
Member

Perhaps you're supposed to use %s there. Check out other warnings in the codebase.

This comment has been minimized.

@gaearon

gaearon Jan 29, 2016
Member

I'd add a note about persist() just before the link so the user doesn't overlook the solution they likely need.

This comment has been minimized.

@kentcdodds

kentcdodds Jan 29, 2016
Author Contributor

👍

warning(
warningCondition,
'This synthetic event is reused for performance reasons. If you\'re ' +
'seeing this, you\'re accessing property `' + propName + '` on a ' +

This comment has been minimized.

@kentcdodds

kentcdodds Jan 29, 2016
Author Contributor

Same lint error here.

@facebook-github-bot
Copy link

@facebook-github-bot facebook-github-bot commented Jan 29, 2016

@kentcdodds updated the pull request.

* These are additional properties not in the EventInterface
* which are nulled when destructoring a syntheticEvent instance
*/
var otherNullableProps = ['dispatchConfig', '_targetInst', 'nativeEvent'];

This comment has been minimized.

@gaearon

gaearon Jan 29, 2016
Member

dispatchConfig and _targetInst are both implementation details and are considered private fields.
I think we don't have to warn on accessing those.

This leaves us with nativeEvent which can be hardcoded as a special case below.

This comment has been minimized.

@kentcdodds

kentcdodds Jan 29, 2016
Author Contributor

👍

@@ -90,17 +100,21 @@ function SyntheticEvent(dispatchConfig, targetInst, nativeEvent, nativeEventTarg
assign(SyntheticEvent.prototype, {

preventDefault: function() {
if (this._destructored) {

This comment has been minimized.

@gaearon

gaearon Jan 29, 2016
Member

As far as I know, in production, this will become an empty branch:

if (this._destructored) {
}

This is why it seems better to wrap the whole thing in __DEV__ rather than the other way around.

This comment has been minimized.

@kentcdodds

kentcdodds Jan 29, 2016
Author Contributor

Ah, yeah, that makes sense.

'https://fb.me/react-event-pooling for more information.'
);
}
return;

This comment has been minimized.

@gaearon

gaearon Jan 29, 2016
Member

Placing return inside __DEV__ means the library can behave differently in development and production, depending on the code below. This can lead to hard-to-reproduce bugs. It’s fine to only log a warning in dev, but all other behavior, including early exits, should be identical.

@@ -195,3 +198,44 @@ SyntheticEvent.augmentClass = function(Class, Interface) {
PooledClass.addPoolingTo(SyntheticEvent, PooledClass.fourArgumentPooler);

module.exports = SyntheticEvent;


// Utility functions

This comment has been minimized.

@gaearon

gaearon Jan 30, 2016
Member

Nit: I don't think this comment is necessary. Do we use similar comments in the codebase?

'This synthetic event is reused for performance reasons. If you\'re ' +
'seeing this, you\'re accessing property `%s` on a ' +
'released/nullified synthetic event. This is %s. See ' +
'https://fb.me/react-event-pooling for more information.',

This comment has been minimized.

@gaearon

gaearon Jan 30, 2016
Member

I still think it's worth adding a sentence about persist() just before the link. If you add it, please do this for every message to keep them consistent.

This comment has been minimized.

@kentcdodds

kentcdodds Jan 30, 2016
Author Contributor

Thanks for the reminder. Forgot about that. I totally agree.

This comment has been minimized.

@gaearon

gaearon Jan 30, 2016
Member

Also it might be good to hoist the almost identical warning message from getter/setter into the function definition, and pass the different part as %s. In addition, it might be best to preserve the old wording (calling a method rather than accessing the property) for methods.

This comment has been minimized.

@kentcdodds

kentcdodds Jan 30, 2016
Author Contributor

In addition, it might be best to preserve the old wording (calling a method rather than accessing the property) for methods.

I considered that, however this wouldn't make sense in a scenario where they're simply getting a reference to the method:

const stop = event.stopPropogation
// maybe use stop later or something?

This would log a warning that wouldn't make sense because they're not actually calling it. I realize that's an edge case, but thought it would make the code simpler and the messaging more accurate.

Definitely willing to be overruled though :-)

This comment has been minimized.

@gaearon

gaearon Jan 30, 2016
Member

Ah, good point. Maybe something like "accessing a method" is neutral enough?

This comment has been minimized.

@kentcdodds

kentcdodds Jan 30, 2016
Author Contributor

That's reasonable. Updating now :-)

On Sat, Jan 30, 2016 at 1:00 PM Dan Abramov notifications@github.com
wrote:

In src/renderers/dom/client/syntheticEvents/SyntheticEvent.js
#5940 (comment):

  •    'This synthetic event is reused for performance reasons. If you\'re ' +
    
  •    'seeing this, you\'re setting property `%s` on a ' +
    
  •    'released/nullified synthetic event. This is effectively a no-op. See ' +
    
  •    'https://fb.me/react-event-pooling for more information.',
    
  •    propName
    
  •  );
    
  •  return val;
    
  • },
  • get: function() {
  •  var warningCondition = false;
    
  •  warning(
    
  •    warningCondition,
    
  •    'This synthetic event is reused for performance reasons. If you\'re ' +
    
  •    'seeing this, you\'re accessing property `%s` on a ' +
    
  •    'released/nullified synthetic event. This is %s. See ' +
    
  •    'https://fb.me/react-event-pooling for more information.',
    

Ah, good point. Maybe something like "accessing a method" is neutral
enough?


Reply to this email directly or view it on GitHub
https://github.com/facebook/react/pull/5940/files#r51350027.

@gaearon
Copy link
Member

@gaearon gaearon commented Jan 30, 2016

At this point I’ve given all feedback I could give, and what I see so far looks good, apart from minor nits above. Let’s wait for the maintainers to give their further comments. Thank you for contributing!

cc @jimfb

@kentcdodds kentcdodds force-pushed the kentcdodds:pr/warn-event-pool-access branch from 105c774 to 69770b3 Jan 30, 2016
@facebook-github-bot
Copy link

@facebook-github-bot facebook-github-bot commented Jan 30, 2016

@kentcdodds updated the pull request.

@kentcdodds kentcdodds force-pushed the kentcdodds:pr/warn-event-pool-access branch from 69770b3 to 113facd Jan 30, 2016
@facebook-github-bot
Copy link

@facebook-github-bot facebook-github-bot commented Jan 30, 2016

@kentcdodds updated the pull request.

@@ -73,6 +73,7 @@ describe('SyntheticEvent', function() {
});

it('should be nullified if the synthetic event has called destructor', function() {
spyOn(console, 'error'); // accessing properties on destructored events logs warnings (tested elsewhere)

This comment has been minimized.

@jimfb

jimfb Feb 11, 2016
Contributor

We should still assert the warnings here. The reason being that we want to know if we start emitting some unexpected warnings. Right now, this test just swallows all warnings.

This comment has been minimized.

@kentcdodds

kentcdodds Feb 11, 2016
Author Contributor

That make sense. I felt odd spying on it and not asserting anything. Will do.

warningCondition,
'This synthetic event is reused for performance reasons. If you\'re seeing this,' +
'you\'re %s `%s` on a released/nullified synthetic event. %s.' +
'If you must keep the original synthetic event around use event.persist().' +

This comment has been minimized.

@jimfb

jimfb Feb 11, 2016
Contributor

coma between "around" and "use"

This comment has been minimized.

@kentcdodds

kentcdodds Feb 11, 2016
Author Contributor

👍

@jimfb
Copy link
Contributor

@jimfb jimfb commented Feb 11, 2016

@kentcdodds Overall, this looks great to me. A couple of nitpicks. Also, I think it would be good to add an "integration" test (ie. render a component, simulate a click event, save the event, read from the event at the end of the test, and assert the warning fires). Just to sanity check that things are working.

Otherwise, I think we're good to merge.

@kentcdodds
Copy link
Contributor Author

@kentcdodds kentcdodds commented Feb 11, 2016

Happy to write the integration test. I haven't looked into how to do that yet, but I'd appreciate it if you could point me in the right direction to do that :-) Thanks for the feedback!

@jimfb
Copy link
Contributor

@jimfb jimfb commented Feb 11, 2016

@kentcdodds A reasonable example is in ReactServerRendering-test.js, we have a test called "should have the correct mounting behavior". Specifically, the most interesting line is: ReactTestUtils.Simulate.click(ReactDOM.findDOMNode(instance.refs.span));

A test would probably look something like this:

var event = null;
var instance = ReactDOM.render(<div onClick={function(e){event = e;}} />);
ReactTestUtils.Simulate.click(ReactDOM.findDOMNode(instance));`
// TODO: assert warnings.length===0
event.nativeEvent;
// TODO: assert warnings.length===1
// TODO: assert warnings[0].contanis("error message text");
@kentcdodds kentcdodds force-pushed the kentcdodds:pr/warn-event-pool-access branch from 113facd to cf76b1c Feb 16, 2016
@facebook-github-bot
Copy link

@facebook-github-bot facebook-github-bot commented Feb 16, 2016

@kentcdodds updated the pull request.

@kentcdodds
Copy link
Contributor Author

@kentcdodds kentcdodds commented Feb 16, 2016

Hi @jimfb. Sorry this took a bit. I've updated the tests and added an integration test as you suggested. I think it's solid. One thing that I did change in response to your comments is I combined two tests. When I asserted the console output in the one test, I realized that it was pretty much identical to another. So I just merged the two into one. Let me know if you'd like to see that change.

Thanks for this opportunity to contribute! :D

@jimfb
Copy link
Contributor

@jimfb jimfb commented Feb 18, 2016

@kentcdodds This all looks good to me, thanks! But it looks like we broke lint (you can run locally with npm run lint or view the output here: https://travis-ci.org/facebook/react/jobs/109724188). Just fix the lint errors and do a "git commit --amend", and we should be good to go.

@jimfb jimfb added this to the 0.15 milestone Feb 18, 2016
@jimfb jimfb self-assigned this Feb 18, 2016
…leased syntheticEvents

Closes #5939
@kentcdodds kentcdodds force-pushed the kentcdodds:pr/warn-event-pool-access branch from cf76b1c to 6312852 Feb 18, 2016
@kentcdodds
Copy link
Contributor Author

@kentcdodds kentcdodds commented Feb 18, 2016

(-‸ლ) thanks! The PR has been updated to fix linting.

Looking forward to my next opportunity to contribute

@facebook-github-bot
Copy link

@facebook-github-bot facebook-github-bot commented Feb 18, 2016

@kentcdodds updated the pull request.

jimfb added a commit that referenced this pull request Feb 18, 2016
Add warning when reading from event which has been returned to the pool
@jimfb jimfb merged commit e8e56e8 into facebook:master Feb 18, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jimfb
Copy link
Contributor

@jimfb jimfb commented Feb 18, 2016

Thanks @kentcdodds!

@kentcdodds
Copy link
Contributor Author

@kentcdodds kentcdodds commented Feb 18, 2016

🎉 🎊

@kentcdodds kentcdodds deleted the kentcdodds:pr/warn-event-pool-access branch Feb 18, 2016
@renovate renovate bot mentioned this pull request Feb 2, 2018
0 of 1 task complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.