Skip to content
This repository has been archived by the owner on Dec 16, 2021. It is now read-only.

Breaking in combination with Material UI #234

Open
linde12 opened this issue Nov 10, 2016 · 59 comments
Open

Breaking in combination with Material UI #234

linde12 opened this issue Nov 10, 2016 · 59 comments

Comments

@linde12
Copy link
Contributor

linde12 commented Nov 10, 2016

Hello!

I am trying to migrate a project from that uses React to Preact.

The project uses the Material UI library by CallEmAll and it doesn't seem to play nice with Preact: Some of their components(like EnhancedButton), call event.persist() on a SyntheticEvent. Instead in Preact this event is a FocusEvent and does not have the .persist() method on it. The result is that Material UI buttons(and probably other components) don't work.

Does anyone know how this could be fixed? Since out whole application is built using Material-UI it is crucial for it to work, otherwise we're unable to migrate.

The SyntheticEvent docs doesn't say much about .persist()(see: https://facebook.github.io/react/docs/events.html) so it's difficult to understand why it's needed and how it could be fixed.

Otherwise, so far, everything else in the migration seems to be working. The components i can see without pressing buttons seem to be properly rendered and work the same way as they would with React.

@developit
Copy link
Member

developit commented Nov 11, 2016

Strange, I've never heard of that method. You could try patching it in:

import { options } from 'preact';
options.event = e => {
  e.persist = () => {};
};

@linde12
Copy link
Contributor Author

linde12 commented Nov 11, 2016

Ah, yes, i did something like similar to that and thought it should work because by looking at the React source it seems to have something to do with persisting the event from their event pool. So i thought it shouldn't affect Preact and it seems like it doesn't. However the button is still not clickable. I think this has to do with FloatingActionButton or any EnhancedButton in material-ui not working with onClick. You have to use onTouchTap or something like onMouseUp.

FloatingActionButton is subscribing to the following events:

  • onMouseDown
    
  • onMouseEnter
    
  • onMouseLeave
    
  • onMouseUp
    
  • onTouchEnd
    
  • onTouchStart
    

I wasn't able to find something similar to react-tap-event-plugin for Preact. Do you have any information about this? As in if it was implemented and should work or if that's something that hasn't been done?

Because the current application(with your fix for the .persist() method) almost everything seems to render properly. I have one problem with a component in the application which causes a crash but i'll narrow that down later. The important thing now is getting all the buttons to work.

Thanks for your help so far @developit

@developit
Copy link
Member

This is really good, I've been hoping to get proper support for Material UI in for some time now. I think we can definitely add the .persist() modification to preact-compat. As for tap-event-plugin, I've actually never used it - I assume that's what is adding support for onTouchTap? Any pointers here would be good, or if you have a repo where you're trying to get this functioning I can play around with that.

@linde12
Copy link
Contributor Author

linde12 commented Nov 11, 2016

Yeah, this combined with #236 will hopefully make MUI run smooth(er) The application i tried this out on was pretty complex and other than the click-events(touch-tap) everything seemed fine.

As for the tap-plugin, yeah it's to make onTouchTap work. This nicely sums up why. As for how to implement it i haven't really looked at it in detail. I know that react-lite uses Fastclick but i'm not entirely sure if this fixes the issue.

No, i don't have any public repo unfortunately but i can set up a small repo which will have the same symptoms. I'm kind of interested in what react-tap-event-plugin does so i will try to look into this as well!

@linde12
Copy link
Contributor Author

linde12 commented Nov 11, 2016

Alright, so after some snooping around it seems like React listens for events on the document which are then passed to the event system which in turn passes it to plugins which in their turn either return null or a SyntheticEvent. Then i'm guessing React just looks at what type of event that is and calls the callback.

So as i understand it:

If a touchstart and a touchend event are < 10px away from eachother and are fired within a small amount of time(a few ms) then it is, to react, counted as a touchTap and if there is a onTouchTap callback it will be called.

We could try one of these approaches:

  • Have a special case for onTouchTap(something like what Preact calls onChange handler when using dispatchEvent while React does not #237 did to onChange) that remaps onTouchTap -> click event and then apply fastclick(get rid of 300ms delay on phones). This is what react-lite does. I'll just have to verify that it actually works.
  • Implement a similar event-system as to what react offers. Maybe we can avoid using synthetic events and keep to the real ones?

@developit
Copy link
Member

Definitely have to go with #1 - synthetic events is one of the things that makes React larger than Preact. I think your first option is pretty good, and the radius detection should be pretty quick to implement. I actually believe I have a demo somewhere of this technique as a preact plugin...

@linde12
Copy link
Contributor Author

linde12 commented Nov 11, 2016

The problem with option 1 however is that components that listen to both onTouchTap and onClick events(see this example, used by all MUI buttons)

  1. Component listens to onClick, name gets mapped to click event in the _listeners object
  2. Component listens to onTouchTap, name gets mapped to click which is already in use so a new listener will not be added
  3. Only the onClick callback is called

React is able to differentiate between these because the underlying events are not click but touchstart, touchend and click

I really think we have to implement onTouchTap using touchstart and touchend to make it work well on both phones and computers.

@developit
Copy link
Member

Likely will have to use a proxy handler that does the type negotiation when the event is triggered. I agree though, my thinking was that a VNode Hook would work for massaging the props to add those multiple listeners to fire the meta-event.

@linde12
Copy link
Contributor Author

linde12 commented Nov 11, 2016

Ah yeah, i was thinking about something similar with the multiple listeners but i'm not too familiar with the code yet. If you solve it i'd like to have a look 👍

@developit
Copy link
Member

Will do, I'll loop you in on the PR!

@stevewillard
Copy link

Any progress on this front? Looking to use preact with material-ui as well.

@linde12
Copy link
Contributor Author

linde12 commented Nov 17, 2016

I have, with a hack, successfully gotten Material UI to work without(afaik) any issues. However i'm hoping developit has a better solution since mine was just a hack to make it work. But to me it seems that we are very close to being compatible with MUI

@developit
Copy link
Member

developit commented Nov 18, 2016

I've been pondering this. I think actually all that is needed is to move this line into the if (opts!==NO_RENDER) block here. Crazy?

@linde12
Copy link
Contributor Author

linde12 commented Nov 18, 2016

@developit

I think you had the wrong issue. This one is about touchtap. Anyway thats what i tried to make refs work but i think i still had problems after doing so because then some refs were not fired. I will come back with results and tag you in the other issue.

Maybe, since there were s few other issues making MUI break, i should just change the title to something about touchtap! :-)

@cia48621793
Copy link

@linde12 Do you had any news? I can get AppBar running by monkeypatching event.persist but I can't get through onTouchTap

developit added a commit that referenced this issue Nov 27, 2016
@developit
Copy link
Member

@cia48621793 I just pushed the persist fix, but I'm wondering if adding onTouchTap will still not fix the issue - won't there still be a broken react-dom/lib/ import?

@harshmaur
Copy link

Didnt know there is an active discussion going on in this. These are still an issue. https://cl.ly/2u2M1x183E2p

@developit
Copy link
Member

@harshmaur the issue is that inject-tap-event-plugin is ridiculously unnecessary with preact-compat, but it relies on 5 internal libs from the react-dom package. We can fake out all the APIs as noops, but it's getting a little silly at that point. Perhaps a replacement for inject-tap-event-plugin would be better, since that's just a single dependency?

@linde12
Copy link
Contributor Author

linde12 commented Nov 28, 2016

@harshmaur @cia48621793 I think it's as @developit says that we need a replacement inject-tap-event-plugin

I don't think it would be difficult for a user to replace that plugin with a preact counterpart, let's say preact-tap

I'm not entirely sure how the API for the event system in preact in terms of hooks looks today but maybe we will need some changes to allow this sort of behavior? What needs to happen is that preact-tap has to listen for touchstart/mousedown & touchend/mouseup and if they're within a certain interval and their positions are close to eachother then it's considered a touchTap so then the plugin would have to trigger a callback in preact

@harshmaur
Copy link

@linde12 I am not sure how replacing would work with material-ui as it tightly integrates with onTouchTap events. The source code also contains their own modifiers like onActionTouchTap events and so on. If there is a plugin that gives onTouchTap support then probably, material-ui would not complain.

@linde12
Copy link
Contributor Author

linde12 commented Nov 28, 2016

@harshmaur Material-UI has a peerDependency towards react-tap-event-plugin which provides the onTouchTap functionality. What i mean is that we could provide the same functionality(without SyntheticEvent) with a new plugin because unfortunately react-tap-event-plugin doesn't integrate well with Preact(the event systems are different). So we would have to make our own replacement plugin.

And onActionTouchTap is just using onTouchTap internally AFAIK so just implementing onTouchTap should solve the issues with MUI.

tl;dr MUI isn't dependent on react-tap-event-plugin per se, but it's dependent on a onTouchTap implementation so we could make one that works for Preact.

@harshmaur
Copy link

@linde12 Totally agreed with your point.

@developit
Copy link
Member

Wouldn't the require('react-tap-event-plugin') throw though? or are they specifying it as a peerDependency just to indicate that you (as the library consumer) need to have installed it?

@harshmaur
Copy link

@developit the latter. material-ui works without react-tap-event-plugin but it wants it to be installed separately.

If there is a custom lib that provides for onTouchTap, IMO material-ui should work fine.

BTW, material-ui@next branch is supposed to work without onTouchTap, however the release is not going to happen any soon.

@developit
Copy link
Member

I wonder - would it suffice to have a package that could be aliased in for react-tap-event-plugin?
It could look something like this:

import { options } from 'preact';
const opts = {
  // # of pixels after which to discount as drag op
  threshold: 5
};

let oldHook = options.vnode;
options.vnode = vnode => {
  let attrs = vnode.attributes;
  if (attrs) {
    let map = {};
    for (let i in attrs) map[i.toLowerCase()] = i;
    if (map.ontouchtap) proxy(attrs, map);
  }
  if (oldHook) oldHook(vnode);
};

function proxy(attrs, map) {
  let start = attrs[map.ontouchstart],
    end = attrs[map.ontouchend],
    tap = attrs[map.ontouchtap],
    coords = e => ({ x: e.touches[0].pageX, y: e.touches[0].pageY });
    down;
  delete attrs[map.ontouchtap];
  attrs[map.ontouchstart || 'onTouchStart'] = e => {
    down = coords(e);
    return start(e);
  };
  attrs[map.ontouchend || 'onTouchEnd'] = e => {
    let up = coords(e),
      ret = end && end(e),
      dist = Math.sqrt( Math.pow(up.x-down.x) +Math.pow(up.y-down.y) );
    if (down && ret!==false && dist<opts.threshold) tap(e);  // or createEvent('touchtap')
    return ret;
  };
}

// in case someone calls inject():
export default (newOpts) => {
  Object.assign(opts, newOpts);
};

@linde12
Copy link
Contributor Author

linde12 commented Nov 28, 2016

@developit

This looks promising! I don't even think you would have to use an alias if unless you don't want to. It's a separate peerDependency so all that would happen is that npm would complain that the dependency isn't satisfied(which it wouldn't be with an alias anyway?)

@harshmaur
Copy link

npm complaining wouldnt be a problem if aliasing works.

@developit
Copy link
Member

Plus this can be a separate package that might actually be useful beyond getting MUI working - anyone who wants basic FastClick style tap events could use it. Not as elegant as a compositional component, but simple once installed.

@cia48621793
Copy link

@developit React-Lite had their solution of touchtap with @fastclick but it won't work with Preact.

@harshmaur
Copy link

harshmaur commented Dec 5, 2016

Okay I tried removing the react-tap-event-plugin and replaced with the sample code. the app kind of loaded, but it doesnt seem to work properly, the elements which do implement onTouchTap arent getting clicked.

@developit
Copy link
Member

developit commented Dec 5, 2016

Yep, sorry - meant remove tap event plugin. Any chance the app you're working on is Open Source so I can check it out and test? :)

It also should have worked when aliased. It seemed like you actually imported it into webpack, really you just want to reference the filename:

resolve: {
  alias: {
    'react-tap-event-plugin': 'path/to/that/file.js'
  }
}

@harshmaur
Copy link

The project is not open source unfortunately.

Also, removing the plugin and using it like this seemed to build the project properly

// Some custom requirement from touch
import reactTapAlternative from './hoc/reactTapAlternative';

reactTapAlternative();

instead of

// Some custom requirement from touch
import injectTapEventPlugin from 'react-tap-event-plugin';

injectTapEventPlugin();

I also tried to resolve however I started getting this.

ERROR in ./src/index.js
Module not found: Error: Cannot resolve module 'function _default(newOpts) {
  Object.assign(opts, newOpts);
}' in /Users/harshmaur/react/mobilenew/src
 @ ./src/index.js 19:27-60

@developit
Copy link
Member

developit commented Dec 5, 2016

That error is very strange - it's trying to load a module called "function _default(newOpts){ ... }" - are you able to post the alias line you're setting in your webpack config? Thanks :)

With the alternative import style it builds, but you dont have tap events, right?

@harshmaur
Copy link

okay, so I did a silly mistake of aliasing a wrong path. However the end result seems to the same.

The app works, but onTouchTap elements dont work.

@developit
Copy link
Member

Alright, I fixed the code sample and published it as preact-tap-event-plugin 👍

Here's a JSFiddle demo of onTouchTap working for both mouse and touch:
https://jsfiddle.net/developit/rq877gp3/

@developit
Copy link
Member

I'm wondering how far we've gotten toward a fully working Material UI now that these two issues are fixed. Anyone know?

@harshmaur
Copy link

Are the transitions working properly? In my case I do not see transitions properly.

Here is the preact version https://cl.ly/3A360X2A0S3e

Here is the react version https://cl.ly/2k473X211Y2b

@developit
Copy link
Member

Hmm - strange. They're not using any translation library, the draw literally just sets properties on .style of an element. I wonder - maybe it's a debounced render triggering after that style is set? Could you try this and see if it fixes it?

import { options } from 'preact';

options.debounceRendering = f => f();

@cia48621793
Copy link

cia48621793 commented Dec 6, 2016

I shamelessly tried to compile the material-ui/docs with preact-compat, shallow-compare and preact-tap-event-plugin with only one single error. To emphasize, it's one single error. Not sure about actual rendering tho.

ERROR in ./~/react-addons-perf/index.js
Module not found: Error: Cannot resolve module 'preact-compat/lib/ReactPerf' in material-ui-master\docs\node_modules\react-addons-perf
 @ ./~/react-addons-perf/index.js 1:17-51

@developit Please fix.

@harshmaur
Copy link

harshmaur commented Dec 6, 2016

Hey @cia48621793, there is a bug in the preact-tap-event-plugin. So it would not work as of yet. But you can try to do it.

The error you are getting is caused due to react-addons-perf so it needs to be aliased with something that is compatible with preact. I am not too sure if there is something already, but you can remove the lib and try to compile.

@cia48621793
Copy link

cia48621793 commented Dec 6, 2016

@harshmaur Thanks for the tip. So far so good. The front page is perfectly rendered. Unfortunately, the preact-tap-event-plugin didn't seems to work and I can't pop the menu up. Looking for a fix, any assistance for me?

@harshmaur
Copy link

@cia48621793 True. @developit is working on the fix, we will soon have it.

@cia48621793
Copy link

@harshmaur Thanks man. It's prestigious to see Material-UI be ran flawlessly so that more and more developers are siphoned to use Preact.

@developit
Copy link
Member

@cia48621793 - is it possible that ReactPerf import error only shows up when running the development server (webpack-dev-server)? I can create an alias, will have to look into what it should do.

@cia48621793
Copy link

@developit No, it was hardcoded in src/app/app.js. Comment out that ReactPerf require and everything goes in the right direction.

@harshmaur
Copy link

@cia48621793 can you put require back and try to build your app instead of starting it?

See if you get the same error or not.

@cia48621793
Copy link

@harshmaur Instruction unclear, Not sure if that's a yay or nay.
In src/app/app.js
Original:

// window.Perf = require('react-addons-perf');

Modified:

window.Perf = require('react-addons-perf');

Then

# npm run browser:build
> material-ui-docs@0.16.4 browser:build C:\Users\Steve\Desktop\material-ui-master\docs
> cross-env NODE_ENV=docs-production webpack --config webpack-production.config.js --progress --colors --profile

36634ms build modules
54ms seal
128ms optimize
46ms hashing
142ms create chunk assets
33022ms additional chunk assets
1ms optimize chunk assets
0ms optimize assets
Hash: ddfd9838654dc3e7900e
Version: webpack 1.13.3
Time: 70039ms
   [0] multi main 40 bytes {0} [built]
       factory:0ms building:2ms = 2ms
    + 1779 hidden modules

ERROR in ./~/react-addons-perf/index.js
Module not found: Error: Cannot resolve module 'preact-compat/lib/ReactPerf' in C:\Users\Steve\Desktop\material-ui-master\docs\node_modules\react-addons-perf
 @ ./~/react-addons-perf/index.js 1:17-51

@developit
Copy link
Member

I just released a noop'd version of lib/ReactPerf as preact-compat@3.9.4

@cia48621793
Copy link

@developit However, I couldn't bring up the menu of Material-UI's documentation website.
Here is my WP config.

  resolve: {
    alias: {
      // material-ui requires will be searched in src folder, not in node_modules
      'material-ui': path.resolve(__dirname, '../src'),
      'react': 'preact-compat',
      'react-dom': 'preact-compat',
      'react-tap-event-plugin': 'preact-tap-event-plugin',
      'react-addons-shallow-compare': 'shallow-compare',
    },
  },

package.json

  "dependencies": {
    "babel-polyfill": "^6.16.0",
    "react-title-component": "^1.0.1",
    "simple-assign": "^0.1.0",
    "preact": "^7.1.0",
    "preact-compat": "latest",
    "preact-tap-event-plugin": "^0.1.2",
    "shallow-compare": "^1.2.0"
  },

IIRC all touchTap related events are not triggered. Please fix.

@cia48621793
Copy link

Strange. I can have material-ui running fine with independent module, I'm sure it's a webpage problem. At least now we can prove material-ui be compromised with Preact and it will attract more people, if not only me.

@harshmaur
Copy link

@cia48621793 What do you mean by independent module?

@cia48621793
Copy link

@harshmaur npm install. I have had it in my another project and MUI works with Preact. Not sure is that because the npm version of MUI is preprocessed with Babel.

@cia48621793
Copy link

@harshmaur @developit Here's my repo proving that Preact is 'working' with Material-UI Docs: cia48621793/preact-material-ui-docs

Try `npm install && npm run start' for a good start. You will soon find errors clicking the item list on the drawer of AppBar. Look for the console error. Please sort them out.

@developit
Copy link
Member

Will check this out. Thanks for setting things up, makes it a lot easier for me to debug! (there are so many of these large react-based frameworks that I don't personally use, it's hard to keep track)

@linde12
Copy link
Contributor Author

linde12 commented Dec 18, 2016

Ah! I've unsubscribed from here by mistake. I just saw that you made the preact-tap-event-plugin repo, great! I will try it out as soon as i get some free time 👍

@SukantGujar
Copy link

We also have a large application which uses material-ui and would be glad to beta test any possible fixes. Please buzz me on any updates you want us to try.

Thanks!

@kerryboyko
Copy link

Would love to update existing codebase to Preact but we designed it on the MUI components. Please buzz me if there are any fixes.

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

No branches or pull requests

7 participants