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

Compatibility with React 16 #122

Closed
Gargron opened this issue Sep 27, 2017 · 10 comments
Closed

Compatibility with React 16 #122

Gargron opened this issue Sep 27, 2017 · 10 comments

Comments

@Gargron
Copy link

Gargron commented Sep 27, 2017

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
Copy link
Author

Gargron commented Sep 28, 2017

@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
Copy link
Member

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
Copy link
Author

Gargron commented Sep 28, 2017

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 🤔

@EtienneLem
Copy link
Member

🤔

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
Copy link
Author

Gargron commented Sep 28, 2017

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
Copy link
Author

Gargron commented Sep 28, 2017

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
Copy link
Member

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

@EtienneLem
Copy link
Member

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
Copy link
Member

EtienneLem commented Sep 29, 2017

🔊 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
Copy link
Author

Gargron commented Sep 29, 2017

✨ 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! 🍰

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

No branches or pull requests

2 participants