Deprecate 'return false' in event handlers #2039

charliermarsh · 2014-08-14T02:47:30Z

Fixes #2015. Wasn't sure how to handle react-version in _config.yml, given that this is a v0.12 milestone.

sophiebits · 2014-08-14T02:48:39Z

docs/_config.yml

@@ -34,4 +34,4 @@ sass:
  sass_dir: _css
 gems:
 - jekyll-redirect-from
-react_version: 0.11.1
+react_version: 0.12.0-alpha


Just leave this alone – docs are built from the 0.11-stable branch and we just won't cherry-pick this in.

sophiebits · 2014-08-14T21:53:15Z

src/browser/eventPlugins/SimpleEventPlugin.js

-   * @param {function} Application-level callback.
-   * @param {string} domID DOM ID to pass to the callback.
-   */
+ /**


the indentation here looks messed up?

charliermarsh · 2014-08-15T15:57:44Z

@spicyj @zpao Fixed! Thanks.

sophiebits · 2014-08-15T19:29:29Z

src/browser/eventPlugins/SimpleEventPlugin.js

+      if (__DEV__) {
+        monitorCodeUse('react_event_return_false');
+        console.warn(
+          'Returning `false` from an event handler will be deprecated in a ' +


one more tweak (sorry): let's say "is deprecated and will be ignored in a future release."

Should it also include === true? For handler() {return bla.shouldReturnFalse;}

Seems reasonable to me.

charliermarsh · 2014-08-15T23:48:22Z

@spicyj @chenglou Updated again.

chenglou · 2014-08-16T00:41:37Z

src/browser/eventPlugins/SimpleEventPlugin.js

@@ -309,6 +310,17 @@ var SimpleEventPlugin = {
   */
  executeDispatch: function(event, listener, domID) {
    var returnValue = EventPluginUtils.executeDispatch(event, listener, domID);
+    if (__DEV__) {
+      if (typeof returnValue === 'boolean') {
+        monitorCodeUse('react_event_return_false');


Sorry, missed this: internally, this is a different function that logs the message. You'd pass a second parameter, which is an object that contains all you need to know to trace back to this function so that people can act upon it:

monitorCodeUse('react_event_return_false', {eventHandlerName: listener.__reactBoundMethod.displayName});

And maybe some other info, dunno. See #2052

FWIW we already have these in the code:

monitorCodeUse('react_object_map_children'); monitorCodeUse('react_no_scroll_event');

I guess the call stack isn't particularly useful though in this case.

Right, and we don't actually bother tracking these two down. These are only to give an idea of the usage pattern internally.

charliermarsh · 2014-08-19T03:01:46Z

@chenglou Fixed. (Note that this now depends on #2052, so the tests won't pass until that's merged in--is there a good way to indicate that?)

chenglou · 2014-08-19T03:06:15Z

Nope. Ill just fix that PR and pull it in quick.

chenglou · 2014-08-19T23:14:01Z

src/browser/eventPlugins/SimpleEventPlugin.js

+    if (__DEV__) {
+      if (typeof returnValue === 'boolean') {
+        monitorCodeUse('react_event_return_false', {
+          eventHandlerName: listener.__reactBoundMethod.displayName


@crm416 ugh sorry, I derped. This is better as (listener.__reactBoundContext && listener.__reactBoundContext.constructor.displayName) || '<<anonymous>>'. It makes more sense to go find the displayName this way, than to look for it on the handler, since the handler can come from a mixin (in which case the method's displayName is kinda useless no matter how we do it).

So if you change this line to that, it won't depend on the other PR anymore, and I can pull this in.

charliermarsh · 2014-08-20T00:56:55Z

@chenglou No problem. Should be fixed now.

zpao · 2014-08-21T00:25:51Z

src/browser/eventPlugins/SimpleEventPlugin.js

+      if (typeof returnValue === 'boolean') {
+        var eventHandlerName = (listener.__reactBoundContext &&
+          listener.__reactBoundContext.constructor.displayName) ||
+          '<<anonymous>>';


this assignment is really tough to read, and I think is better written with a ternary

var eventHandlerName = listener.__reactBoundContext ? listener.__reactBoundContext.constructor.displayName : '<<anonymous>>';

Though I suppose that capture the case where listener.__reactBoundContext.constructor.displayName is undefined...

var eventHandlerName = (listener.__reactBoundContext && listener.__reactBoundContext.constructor.displayName) || '<<anonymous>>';

is slightly better I suppose...

My bad. Also, would a component name be enough? I guess it is?

charliermarsh · 2014-08-21T01:14:13Z

@zpao Fixed! Thanks.

chenglou · 2014-08-21T21:15:13Z

@crm416: @sebmarkbage has spoken. You can take out that monitorCodeUse completely; I just tested internally and the important things don't break, so we probably don't need to track that anymore. Sorry for the back and forth!

charliermarsh · 2014-08-22T05:10:54Z

@chenglou No worries--monitorCodeUse removed.

chenglou · 2014-08-22T17:17:34Z

src/browser/eventPlugins/SimpleEventPlugin.js

@@ -309,6 +309,16 @@ var SimpleEventPlugin = {
   */
  executeDispatch: function(event, listener, domID) {
    var returnValue = EventPluginUtils.executeDispatch(event, listener, domID);
+    if (__DEV__) {
+      if (typeof returnValue === 'boolean') {


There's a warning module (similar to invariant. Use that one instead.

charliermarsh · 2014-08-23T01:47:54Z

I noticed that in ReactPropTransferer, we have a didWarn flag to avoid printing the "transferPropsTo is deprecated" warning excessively. Seems like that would be useful here too?

chenglou · 2014-08-23T01:50:47Z

@sebmarkbage ^
I think this should be ok? It's kinda hard to track and there's apparently no important users of return false anyways.

Deprecate 'return false' in event handlers

Deprecate 'return false' in event handlers

45f8b52

sophiebits reviewed Aug 14, 2014
View reviewed changes

Re-add with warning and monitorCodeUse

a195d3e

sophiebits reviewed Aug 14, 2014
View reviewed changes

Fixed style and comment nits

c5be746

sophiebits reviewed Aug 15, 2014
View reviewed changes

Test for both true and false

429650a

chenglou mentioned this pull request Aug 16, 2014

Give displayName for all compositeComponent spec functions #2052

Closed

chenglou reviewed Aug 16, 2014
View reviewed changes

More info in monitorCodeUse call

cf45a40

chenglou reviewed Aug 19, 2014
View reviewed changes

Look on __reactBoundContext for displayName

0ecc7e5

chenglou added the Internal diff created label Aug 20, 2014

zpao reviewed Aug 21, 2014
View reviewed changes

Indentation for eventHandlerName

f01246d

chenglou mentioned this pull request Aug 22, 2014

Including missing validation check #2078

Merged

Removed monitorCodeUse call

979a93a

chenglou reviewed Aug 22, 2014
View reviewed changes

Use warning module

eb36b57

chenglou added Internal diff accepted and removed Internal diff created labels Aug 27, 2014

chenglou added a commit that referenced this pull request Aug 27, 2014

Merge pull request #2039 from crm416/return-false

74ff6fb

Deprecate 'return false' in event handlers

chenglou merged commit 74ff6fb into facebook:master Aug 27, 2014

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate 'return false' in event handlers #2039

Deprecate 'return false' in event handlers #2039

charliermarsh commented Aug 14, 2014

sophiebits Aug 14, 2014

sophiebits Aug 14, 2014

charliermarsh commented Aug 15, 2014

sophiebits Aug 15, 2014

chenglou Aug 15, 2014

charliermarsh Aug 15, 2014

charliermarsh commented Aug 15, 2014

chenglou Aug 16, 2014

sophiebits Aug 17, 2014

chenglou Aug 17, 2014

charliermarsh commented Aug 19, 2014

chenglou commented Aug 19, 2014

chenglou Aug 19, 2014

charliermarsh commented Aug 20, 2014

zpao Aug 21, 2014

zpao Aug 21, 2014

chenglou Aug 21, 2014

charliermarsh commented Aug 21, 2014

chenglou commented Aug 21, 2014

charliermarsh commented Aug 22, 2014

chenglou Aug 22, 2014

charliermarsh commented Aug 23, 2014

chenglou commented Aug 23, 2014

Deprecate 'return false' in event handlers #2039

Deprecate 'return false' in event handlers #2039

Conversation

charliermarsh commented Aug 14, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charliermarsh commented Aug 15, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charliermarsh commented Aug 15, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charliermarsh commented Aug 19, 2014

chenglou commented Aug 19, 2014

Choose a reason for hiding this comment

charliermarsh commented Aug 20, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charliermarsh commented Aug 21, 2014

chenglou commented Aug 21, 2014

charliermarsh commented Aug 22, 2014

Choose a reason for hiding this comment

charliermarsh commented Aug 23, 2014

chenglou commented Aug 23, 2014