[Core] Add react-tap-event-plugin to components that need it

[mbrookes]

Any reason we can't just add react-tap-event-plugin to the components that need it? It seems like this would be the simplest way to solve this recurring issue!

[alitaheri]

I did a bit of digging And it seems react doesn't allow plugins with the same name to be registered more than once here And since we have module caching, it's safe to inject the react-tap-event-plugin more than once, making this solution reliable.

I personally like this solution as we can reduce a huge amount of noise in issues. and the only overhead would be attempting to inject just a couple of times more, which is really negligible.

👍 For this awesome solution 😁

BUT! react-tap-event-plugin must remain a peer dependency! as there should be only one instance to not trigger the warning.

[newoga]

After starting #3064, I also wondered if it'd be easy enough to just implement what @mbrookes suggested here. My only thought was maybe it would be too magical and it'd be a change we'd have to consider carefully. I'm definitely open to this approach though if everyone agrees.

My immediate but not terribly pressing (and definitely debatable) concerns were:

1. Should material-ui really be automatically calling code that affects React internal state? Maybe there are side effects we aren't considering.
2. Do we want to couple react-tap-event-plugin to material-ui? For example, if we leave it as is, if Facebook does eventually decide to include it in core or make it an addon, existing material-ui users really just need to switch their package.json and import. If we couple it, it means they need to update to newest version of material-ui.

[mbrookes]

My thoughts on your concerns:

1. Material UI doesn't function without react-tap-event-plugin, so I'm not clear what side effects this could have that including it at a project level doesn't already (unless I'm misunderstanding).
2. It's supposed to come with React 1.0, which is almost certainly going to have other breaking changes that will require users to update the version of Material-UI anyway. Even if it doesn't, I really don't see it as a major problem to tie the two releases at that single point in time.

[alitaheri]

I agree with @mbrookes On this one. the version 1.0 is going to take a lot of time. and it will be like 2 lines per component page. easily removable with find and replace.

P.S. It doesn't change the internal workings, it fixes them. 😁

[oliviertassinari]

What about people how are using material-ui as a standalone package, with a CDN for instance ?
I have now idea if this would work. I remember some issues regarding this point. People had to use some hack to make it work.

Otherwise, I like the @mbrookes proposal 👍.

[mbrookes]

I didn't think that was possible at the moment, but even if so, surely it's no different than the React peer dependency?

[alitaheri]

guys do you think doing this will fix #1192 or make it worse 😁

[newoga]

Mm, I'm not sure, my guess it wouldn't help or hurt 😄. It still seems like some people have an issue with onTouchTap even though they are injecting the plugin. That's another one of my general concerns and I'm not sure the best way to tackle it.

[alitaheri]

Yeah you have a point O.o

This is related to react. I'm ok with both approaches. as they both reduce the noise. I would say let's implement @mbrookes idea and if we see complains in the release candidates. we'll go back to @newoga idea. 😁

[newoga]

Sounds good to me 👍 I'm good with this approach, though haven't settled on an implementation yet.

I did a bit of digging And it seems react doesn't allow plugins with the same name to be registered more than once here And since we have module caching, it's safe to inject the react-tap-event-plugin more than once, making this solution reliable.

I think more components need this plugin than not. Do you think it'd be alright to just call it in getMuiTheme()? Pretty much every component depends on that function and it will only call the function if theme is undefined in context and state, so practically it should only get called once even though it's not an issue if gets called multiple times. I think this is the easiest way to implement this and only affects one file.

[alitaheri]

Well getMuiTheme will be called when the top level render is called on the application that uses it. I don't know if injecting that plugin while a render is in order would have implications or not.

[newoga]

Well getMuiTheme will be called when the top level render is called on the application that uses it. I don't know if injecting that plugin while a render is in order would have implications or not.

Yeah, I'm not sure either. I tested it an it appears to work (it's in #3079). But maybe we should investigate whether there are other issues that I'm not considering.

[alitaheri]

Wow that was fast 👍 👍 Thanks a lot 😁

[newoga]

So there was just an update to the react-tap-event-plugin a few days ago that allows the threshold in which onClick events are fired after detection of touch events to be configured.

https://github.com/zilverline/react-tap-event-plugin#ignoring-ghost-clicks

I'm not saying we shouldn't pursue this idea anymore, but this is an example of why coupling this plugin in our library might cause issues. 😁 For example, the current implementation in #3079 calls injectTapEventPlugin() without exposing the the configurability that was recently added. Just telling users that they should call injectTapEventPlugin themselves is just one less thing we have to follow/track and watch for changes.

[alitaheri]

Damn! We should definitely go back to the warning now -_-

I didn't expect to see a configuration on that thing :D :D

@mbrookes Unfortunately we can't implement this anymore.

[mbrookes]

Didn't see that coming, but it's no big deal. If a user wants to use that feature, they just call injectTapEventPlugin themselves before any Material-UI components.

#3079 doesn't break anything.

[newoga]

Didn't see that coming, but it's no big deal. If a user wants to use that feature, they just call injectTapEventPlugin themselves before any Material-UI components.

Yeah we'll have to look into this more. Looking at source I think if they called injectTapEventPlugin themselves before material-ui, the implementation in #3079 would either throw a warning (because React would detect the plugin being passed would be different than what is there already) or the new one (ours) would just overwrite the one the user passed first.

If we pursue this, we may have to use a combination of #3079 and #3064 to check if the plugin is already registered before calling injectTapEventPlugin ourselves.

[newoga]

@mbrookes Unfortunately we can't implement this anymore.

There still be may be a way to do this properly and safely, and we can continue to investigate. But I just think features like this have to be considered and implemented very carefully. For example, developers have ran into trouble when auto-prefixing was not configurable. I just want to make sure we don't make same mistake with different libraries 😁.

[mbrookes]

If i'm reading the injectTapEventPlugin source correctly, it doesn't look like the name changes if you provide params.

Worth double-checking, but I think we're good.

[newoga]

@mbrookes Yeah ultimately we're probably just going to have to test it to confirm. But it seems like React plugin hub isn't just using the name to check, it's also using the Plugin instance. Since the injectTapEventPlugin is really a function computing a new function based on the configuration, I imagine it would break.

But even so, if it doesn't throw the warning, React overwrites the existing plugin with the new one passed. So users calling it first won't solve the problem.

[mbrookes]

Hm, yes, I see what you mean. Well then a combination of your two approaches seems like a good strategy. Over to you! 😄

[newoga]

Haha yes, I'll give that a shot and update the PR 😄

[prewk]

Hi, I've suddenly started getting this error, is it related to this issue?

edit: It only seems to happen when I require my React application in node with babel-node, and not when webpack's bundling it. I understand if that's a weird edge case, but I'm doing it for server-rendering.

edit2: My edge case gets narrower and narrower, my problem seems to be that I'm using require-new (in development) for it to reload the modules without restarting the server, and this is causing the error. So never mind me.

[SPAHI4]

+1

Uncaught Invariant Violation: EventPluginRegistry: Cannot inject two different event plugins using the same name, `TapEventPlugin`

[newoga]

I think we've decided against doing this automatically but I'm still open to implementing a warning.