missive / emoji-mart

🏪 One component to pick them all
https://missiveapp.com/open/emoji-mart
MIT License
8.59k stars 826 forks source link

Compatibility with React 16 #122

Closed Gargron closed 7 years ago

Gargron commented 7 years ago

Error: Element ref was specified as a string (search) but no owner was set. You may have multiple copies of React loaded. (details: https://fb.me/react-refs-must-have-owner).

The code uses string-based refs in multiple places, I believe. That functionality was marked for deprecation in React 15. React 16 was just released, and no longer supports this feature.

Gargron commented 7 years ago

@EtienneLem Is it possible for you to release an updated version on npm soon? I know not all of the bugs that I found are fixed yet, but I'm excited to upgrade Mastodon to React 16 asap

EtienneLem commented 7 years ago

Just released v2.0.0 (changelog) 🎆

Let me know if you hit any rough edges while importing into your app, hopefully we haven’t overlooked anything 😅

Bug fixes coming soon ✌️

Gargron commented 7 years ago

Welp, after upgrading I immediately got this error on compile:

ERROR in ./node_modules/emoji-mart/data/index.js
Module not found: Error: Can't resolve '../src/utils/build-search' in '[PATH STRIPPED]/node_modules/emoji-mart/data'
@ ./node_modules/emoji-mart/data/index.js 1:248-284

and

ERROR in ./node_modules/emoji-mart/dist/components/picker.js
Module not found: Error: Can't resolve 'measure-scrollbar' in '[PATH STRIPPED]/node_modules/emoji-mart/dist/components'
@ ./node_modules/emoji-mart/dist/components/picker.js 41:24-52

I wonder if that's my fault somehow :thinking:

EtienneLem commented 7 years ago

🤔

Well '../src/utils/build-search' surely is wrong, since we should be getting that in dist. measure-scrollbar I (blindly) moved that into devDependencies… but since we’re not exactly bundling anymore I guess we do need to require it at runtime.

Will be fixing that shortly.

Gargron commented 7 years ago

Are we not bundling? I didn't change the Webpack config much. Actually there's an argument to be made about using Rollup instead of Webpack since this is a library and could benefit a lot from tree-shaking and that stuff, but I opted to not suggest that because developing this component with storybook is much more convenient.

@nolanlawson might know more than me on this.

Gargron commented 7 years ago

Regarding the ../src thing, I didn't change that part of the config but maybe path.resolve(__dirname, './') style like in the storybook one is better than path.resolve('src/')?

EtienneLem commented 7 years ago

No worries, quickly fixing everything (my bad, tried to release too fast) and will explain everything after 😄

EtienneLem commented 7 years ago

FYI: Was realllllly close to fixing everything (turns out there was a few more things), but I really do need to go now. Taking the plane later tonight, I might get a chance to finish during the flight. (sorry)

EtienneLem commented 7 years ago

🔊 AIGHT! v2.0.1 should be working 🤞

So, about that bundle thing: We indeed do not bundle anymore since v2.0.0 (#105, #108). Greatly reduces the size of the lib (which is most likely one of the biggest dependency of most people because of the emojis data 👎) and I believe developers aren’t affected by that in the end. For example, in Missive we’re using Webpack but with CoffeeScript (no Babel). The way dist is now built (CommonJS), just using plain webpack without any config should work. Updating to v2.0.1 was plug and play for us, no need to setup any extra loaders even if it’s not bundled anymore.

That is why measure-scrollbar had to be in dependency (although I did copy the lib locally and removed the dependency).

Gargron commented 7 years ago

:sparkles: It works! My PR to upgrade Mastodon to React 16 is now in a working state. There are still some bugs that occur with this component that I created issues for, but it's looking good! :cake: