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

Loosen react dep allowing 16+, add ReactDOM to externals #11

Closed
wants to merge 15 commits into from
Closed

Loosen react dep allowing 16+, add ReactDOM to externals #11

wants to merge 15 commits into from

Conversation

apuntovanini
Copy link
Contributor

This can be a WIP for allowing React 16. Tests passed without problems, still need to check in some concrete examples, but no big issues so far.
Am I missing something? Seemed too easy, there must be something I am missing out 😄

@apuntovanini
Copy link
Contributor Author

Ok, I can confirm: I tested with React 16 on multiple inputs and it works. One of the issues that may arise is (and that happened to me was) TypeError: this.updater.enqueueCallback twisty/formsy-react-components#118
The issue is related to mismatching versions of React, and not related to Formsy.
A non blocking issue I found out is related to #4 on select controls

@apuntovanini
Copy link
Contributor Author

apuntovanini commented Nov 10, 2017

This fixes #7

@apuntovanini apuntovanini changed the title Loosen react dep allowing 16+, add reactDOM to externals Loosen react dep allowing 16+, add ReactDOM to externals Nov 10, 2017
@rkuykendall rkuykendall mentioned this pull request Nov 10, 2017
@apuntovanini apuntovanini requested review from twisty, MilosRasic, rkuykendall and aesopwolf and removed request for twisty November 13, 2017 09:30
@rkuykendall
Copy link
Member

From what I can tell, this looks good. I'll try to get it running on my project again today. One odd thing is the release folder being included in the PR. Should we do that? Seems like it would be fragile in merges. I imagined we would only update that in release commits where we updated the version numbers, but I don't know.

@apuntovanini
Copy link
Contributor Author

Nope, you're right, release folder shouldn't be in the PR unless we want to let users try importing UMD versions. Lib folder instead should be included, since it targets es5 projects, and it's needed in many cases, but thats my opinion. We can allow both using package.json, whats your idea?

@MilosRasic
Copy link
Contributor

MilosRasic commented Nov 13, 2017

We should put whatever package.json main is pointing to (currently lib) in the repo if we want people to be able to use it with github urls in package.json. Currently npm install git+https://github.com/formsy/formsy-react.git doesn't work because lib is missing.

@apuntovanini
Copy link
Contributor Author

@MilosRasic if you use it from repo directly yes, you'd need lib folder. We should keep it in the repo until the React 16 support PR (1.1.0) in order to allow us to test it from git directly. But later on we'll need to remove it and make it available only in the npm package, not in repo. I added it back in this PR, so you can test it

@MilosRasic
Copy link
Contributor

@apuntovanini Thanks. I'd also like to test with 1.0.1 before switching to the React 16 branch. Can someone please release 1.0.1 on npm? @rkuykendall, @twisty ?

@apuntovanini
Copy link
Contributor Author

apuntovanini commented Nov 13, 2017

If we release master branch we won't have access to lib anyway, can you send a PR for the lib folder added in the repo? This PR needs more tests to land in master, so we need another @MilosRasic

@rkuykendall
Copy link
Member

@apuntovanini @MilosRasic Looks like I have NPM access. I'll create a PR to add lib and bump the version number.

@apuntovanini
Copy link
Contributor Author

apuntovanini commented Nov 13, 2017 via email

@rkuykendall
Copy link
Member

I don't think so? Is one of these you? I'm actually not that familiar with how NPM manages permissions.

npm access ls-collaborators formsy-react
{
  "rkuykendall": "read-write",
  "sdemjanenko": "read-write",
  "twisty": "read-write",
  "track0x1": "read-write",
  "st-andrew": "read-write",
  "aesopwolf": "read-write",
  "semigradsky": "read-write",
  "christianalfoni": "read-write"
}

@rkuykendall
Copy link
Member

I bumped the version number and ran yarn build but the src folder didn't change. Release folder did. #16

@apuntovanini
Copy link
Contributor Author

Ok, no 😢 I don't have access. I'll ask @aesopwolf to add me if I gained enough trust 😄

@rkuykendall
Copy link
Member

@apuntovanini Did the rebase go sideways? It seems like the PR now contains a lot of commits / changes that have already been applied to master, which is odd.

@apuntovanini
Copy link
Contributor Author

Ok, I may’ve got lost in this, but as of my understanding now the PR is aligned with 1.0.2 and the merge would work. But to be honest I always get lost in PR that should be realigned with master... what do you suggest?

@MilosRasic
Copy link
Contributor

Ok, I've tested the 1.0.2 with https://github.com/gogoair/react-formsy-combo-select with the latest version of React installed. For some reason my version of npm didn't duplicate React even though formsy-react requires ^15.6.1. Everything works fine, no warnings or other issues. Can we resolve the rebase issue and merge tihs? I'm afraid I can't help much since I don't have much experience with rebasing. @apuntovanini maybe you can just squash some of the commits into one?

Either way, it will take some time to move my multi-repo project at work to React 16, and I'm on vacation, so I wouldn't like to hold this back. I'm in favour of just publishing 1.1.0 with React 16 support at this point. If we find something wrong we can always quickly publish a fix or two.

@apuntovanini
Copy link
Contributor Author

@MilosRasic I think I'll close this PR in favour of a new one tonight, so that we can merge this. I agree with a 1.1.0 release, we can fix it either way if issues arise, and if we didn't release it we'd never find out 😄 , so, yes, I'll be working on it tonight and we'll be ready to go!

@rkuykendall
Copy link
Member

If we find something wrong we can always quickly publish a fix or two.

Yeah, we're going to make mistakes, but I think that's better than being frozen by doubt. If we just keep this project moving and very responsive to feedback / bug reports and we should be fine.

@apuntovanini apuntovanini mentioned this pull request Nov 15, 2017
@apuntovanini apuntovanini deleted the react16-support branch November 15, 2017 20:12
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.

3 participants