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

Upgrade to React 16 #196

Merged
merged 7 commits into from
Mar 8, 2018
Merged

Upgrade to React 16 #196

merged 7 commits into from
Mar 8, 2018

Conversation

puffo
Copy link
Contributor

@puffo puffo commented Feb 10, 2018

Hi there!

This upgrades the project to React 16 (#189).

Enzyme 3 requires that we re-find elements that might have changed (see this discussion). It has resulted in some of the tests looking a bit clunkier in that we unfortunately can't instantiate all the necessary variables at the start of the tests.

I'll need some help to finish it up though - in attempting to merge the latest changes from #195, I can't seem to get the most recent test passing.

Any suggestions/help?

@patw0929
Copy link
Owner

Thank you for this PR! I've leave some comment above.
Hmmm... however I have no idea about the failed tests too.
Need some time to figure out.

@ignatiusreza
Copy link
Contributor

Hi guys, sorry for bumping this issue.. anything I can do to help move this PR forward? would be nice to have this library be react 16 compatible, since it is currently the last package holding us from upgrading to react 16..

@puffo
Copy link
Contributor Author

puffo commented Mar 8, 2018

Hey @ignatiusreza, I was stuck after updating this branch (from master) to resolve the merge conflicts. I couldn't get the changes from #195 working in the test suite.

Would you be able to try on your side?

@ignatiusreza
Copy link
Contributor

managed to make it pass with react-16 https://github.com/ignatiusreza/react-intl-tel-input/tree/upgrade/react

@puffo do you mind if I open another PR?

@puffo
Copy link
Contributor Author

puffo commented Mar 8, 2018

Thanks for your help @ignatiusreza! I've pushed up the changes and cleaned up as per the changes on your fork.

I'm now struggling with the Travis CI build, but it looks like it the build isn't pulling from my branch.

Not sure if you can help out here @patw0929 or @ignatiusreza ?

@patw0929
Copy link
Owner

patw0929 commented Mar 8, 2018

@puffo @ignatiusreza Thank you bros! Awesome!
About the Travis CI build fail, could you refer this 92df903 to see if works?

@coveralls
Copy link

Coverage Status

Coverage remained the same at 94.02% when pulling fdff11c on puffo:upgrade-to-react-16 into 4d0f479 on patw0929:master.

@puffo
Copy link
Contributor Author

puffo commented Mar 8, 2018

Build is now green @patw0929 ! Thanks!

@patw0929 patw0929 merged commit d702ee4 into patw0929:master Mar 8, 2018
patw0929 added a commit that referenced this pull request Mar 8, 2018
Breaking change:

* #196 Upgrade to React 16 (by @puffo & @ignatiusreza)
"npm": "^5.6.0",
"raf": "^3.4.0",
"react": "^16.2.0",
"react-dom": "^16.2.0",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry for the late comment.. but I think these dependencies should not be forced unto the users:

  • react and react-dom is already mentioned as peerDependencies,
  • npm is not used, not sure why it's there in the first place
  • enzyme-adapter-react-16 is only used for development, and user does not necessarily test their code using enzyme
  • raf is not the only option for polyfilling requestAnimationFrame.. user might already do it through other method.. and if react them self doesn't force one particular way to polyfill it, I don't think that we should too..

opened a PR to fix these points: #199

andrewsantarin pushed a commit to andrewsantarin/react-intl-tel-input that referenced this pull request Feb 2, 2022
andrewsantarin pushed a commit to andrewsantarin/react-intl-tel-input that referenced this pull request Feb 2, 2022
Breaking change:

* patw0929#196 Upgrade to React 16 (by @puffo & @ignatiusreza)
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.

4 participants