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

Fix NodeList to Array convertion #260

Merged
merged 2 commits into from
Feb 17, 2017

Conversation

huumanoid
Copy link
Contributor

Related to issue #256

Hope this PR would fix it once and for all.

Thanks @LeoIannacone for his diagnostics and issue report.
Thanks @tasiek for his extremely helpful diagnostics and feedback about my patch.
Thanks to @gauthierj for his important feedback, I'm glad that this approach OK for him.

Thanks everyone who has faced this issue and test this fix to make sure problem it gone.

npm install 'huumanoid/react-tooltip#fix-undefined-is-not-a-function'

Discuss:
Should this library directly depend on fbjs? react-dom is already depends on fbjs.

@wwayne
Copy link
Collaborator

wwayne commented Feb 17, 2017

@huumanoid Thanks a lot :), but the CI failed for Error: Cannot find module 'fbjs/lib/createArrayFromMixed', do we need to add fbjs as dependencies then?

@huumanoid
Copy link
Contributor Author

Thanks for your response!
The situation is quite hard.
As mentioned at https://github.com/facebook/fbjs, it's not reliably to depend on fbjs. They don't follow semver strictly.
Both legacy and modern react are depend on fbjs.

I see three possible solutions.

  1. Just add fbjs to dependencies. Con: it may break at any time.
  2. Add strict dependency on the exact current version. Pro: breakage is less likely, however, is still possible, I guess. Con: when they update fbjs, we will have at least two copies of fbjs in node_modules (one for react-tooltip and one for react-dom. Who cares?
  3. Just copy&paste required code to this project. Pros: reliably and lightweight. Cons: Copypasting of the already available code? Err... Moreover, are licenses compatible?

I can't do such a decision by myself.
Copy&paste is easy to do, required code has zero dependencies.
I trust in your wisdom. What should be do in this situation?

@wwayne
Copy link
Collaborator

wwayne commented Feb 17, 2017

@huumanoid I prefer to check their solution first and write it by ourselves (well it's like copy paste, haha). I can do it in these 2 days.

Thanks again, your comment is really helped.

@wwayne wwayne merged commit 661a384 into ReactTooltip:master Feb 17, 2017
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

Successfully merging this pull request may close these issues.

2 participants