-
Notifications
You must be signed in to change notification settings - Fork 434
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
ES6 Classes and Higher Order Components #439
base: master
Are you sure you want to change the base?
ES6 Classes and Higher Order Components #439
Conversation
First glance this looks fine. Ideally we will rewrite all the docs (and source) to use ES6 classes and High Order Components though. #395 I'll wait a week (maybe 2?) before merging this. Hopefully myself, or someone else can create a PR to fully deprecate createClass/createReactClass |
If that's the plan it should be no problem for me to kill 2 birds with one stone and do that here :) the question is what to do with the mixin related code though: should that be removed along with createClass, so only the higher order component remains? I'm happy to change this PR to do that (which fixes the warnings anyway). |
Ok, here's the beginning of it, all but 10 or 11 tests are passing. Once they're fixed then we can see if anything in there should be cleaned up. :) |
Oh this would be fantastic! |
It's been taking a teensy bit longer than I planned, sorry :( |
@leemachin Can I help expedite this somehow? |
@track0x1 there are just the last ~10 failing tests, some of which may be due to the testing utilities (that all relied on mixin behaviour previously), and others because I started moving the handlers into props. e.g. the two main failures are this:
That's likely to be a render method in one of the test components. The stack trace reveals nothing, though, so it's difficult to pinpoint.
That one's pretty easy to debug. :) |
Okay, apart from the documentation and examples (simple find and replace changes I guess) the last question is how to deal with all of the handlers that were once mixed in. Since they're all props now they have to be extracted before the main elements are rendered, otherwise React will complain. The Those are the final two problems. |
any updates here? since atm this lib will not work with fiber due to the mixin i suspect |
@leemachin @aesopwolf |
@samuv that would help, I think. It feels more and more like a rewrite as I try to make changes; it's too much to do all at once. |
@leemachin Yep, it feels like a rewrite to me, if we move everything on the specific branch we can focus on each problem with dedicated pull request and isolate all the things that we need. How can we move forward with this approach? |
@leemachin @samuv - I've created a PR to @leemachin's branch that fixes the remaining tests. |
Thank you @st-andrew, the only thing I don't understand if there is an active maintainer in this thread that can help us to create a doable workflow for this release. It would nice to have the possibility to create a project here in this repo for understanding the effort and creating some tasks for what we need. |
Yea because of the size of these changes, a beta release would be good to catch any breaking behavior. I hope there's still a maintainer kicking around to review these changes, otherwise might have to fork and republish under a different name to carry on (but i'd rather contribute to this repo). |
even if this is forked it'd be awesome because currently this lib is preventing me from updating to 15.5.4 :( these updates should allow for me to update |
I’d be more than happy to point to a forked version, even if it was temporary. |
@brandondurham please/thanks |
@brandondurham Have you had a chance to fork this? I'm reviewing other form libraries, but I haven't found any that are as easy to use as Formsy. |
@bigblind - I'm almost done my fork. Hoping to have it published this week. |
I've published a new fork to npm called formsy-react-2. Make sure you read the README, there are definitely areas for improvement and I will gladly accept PRs. I've also published a new package based on formsy-material-ui called formsy-mui that makes use of formsy-react-2 if you're looking for examples. Eventually I'll update the documentation and add examples, for now I just wanted to get these released. |
Currently WIP
Fixes Deprecation of React.createClass #437, Deprecation of React.PropTypes #438 and Rewrite readme mixin example to use Higher Order Component #395 by updating React internally to 15.5, and replacing
createClass
with ES6 classes.Converts the
Mixin
to a HOC, because of the reasoning in Rewrite readme mixin example to use Higher Order Component #395 and because it won't work withoutcreateClass
anyway. This means thatMixin
needs a new name.Passing tests
Optionally (depending on feedback from other collaborators because it increases the PR size):
require
for importing,var
overconst
, etc.)