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

2.0.0 #77

Closed
MilosRasic opened this issue Feb 13, 2018 · 6 comments
Closed

2.0.0 #77

MilosRasic opened this issue Feb 13, 2018 · 6 comments

Comments

@MilosRasic
Copy link
Contributor

Hi, fellow contributors. I feel the time has come to start considering a new major version release so that we can introduce some breaking changes and deal with issues that burden formsy from the old times when things were handled with mixins. Let's post ideas for 2.0.0 here as an "umbrella issue" so that we can start working on them and hopefully release a beta at some point. Also feel free to tell me that I'm dumb and we don't need a new major version ;)

For the starters, we have #48 and #50, both reviewed and awaiting merge, both with introducing breaking but much needed changes.

To start with ideas, one that I had is streamlining withFormsy. Currently, for each custom form element, we need to make a new component that renders the base one, an error message and copies the value to Formsy HOC's state. This is pretty simple but there's a lot of code, a lot of boilerplate to just copy-paste around. I'd like to improve this by making withFormsy work more like Redux's connect. Something like:

import { withFormsy } from 'formsy-react';
import React from 'react';
import MyInput from './some/path/MyInput';

const mapValue = event => {
  return event.currentTarget.value;
};

const renderWrapper = (Component, props) => (
  <div>
    <Component {...props} />
    <span>{this.props.errorMessage}</span>
  </div>
);

export default withFormsy(MyInput, mapValue, renderWrapper);

We could also make mapValue and renderWrapper from this example the defaults, so it would simply look like

import { withFormsy } from 'formsy-react';
import React from 'react';
import MyInput from './some/path/MyInput';

export default withFormsy(MyInput);

and uses could still supply their own mapValue and renderWrapper for cases that differ from defaults. Should probably also allow onChange prop name override for base components that deviate from the standard.

This has multiple benefits over the current HOC:

  1. Much less verbose, especially for simple cases.
  2. Easier to just drop into whatever file is using Formsy rather than writing a separate file to wrap each custom component.
  3. Looks cool af. I mean, that's a render prop as a HOC argument... a render argument?

Another idea would be performance issues. Aside from optimizing existing code if feasible, I'd also like to look into the possibility of marking elements as "isolated", meaning they don't affect rendering and validation of the form and other elements. With elements like these, we could opt to not rerender the form when they change. It would probably have to be calculated at runtime and not left to user to specify as a prop.

Any of this make sense to you guys or am I just rambling?

@Kaishiyoku
Copy link

Kaishiyoku commented Feb 14, 2018

Your suggestions make sense to me and I would appreciate it to seeing a 2.0.0 release in the future.

@rkuykendall
Copy link
Member

Your suggestions are interesting, but looking through my Formsy components, they are all pretty beefy and unique. You're saying the old functionality would still work, right? This seems specifically for very simple inputs.

@rkuykendall
Copy link
Member

As far as 2.0.0, I'm down to go in that direction. #48 is a great change. #50 needs documentation and testing. But that could be done more easily if it were part of a 2.0 development push.

How does this sound for a plan:

  1. Merge both
  2. Update version number on master but don't do a release
  3. Make more proposal PRs off master, like this change, update documentation, etc.
  4. When everything is settled and stable on master ( ideally I'm running it in prod ), release a 2.0.0

How does that sound?

@MilosRasic
Copy link
Contributor Author

I wouldn't do it on master. If we find a bug that could be fixed in 1.x it would become impossible if master is bumped to 2.0. How about a 2.0 branch which we can merge into master once it's good enough for release?

@rkuykendall
Copy link
Member

True. The only downside to a branch is that last time I wasn't able to get it to download as part of a package.json for testing. I'm sure somebody here could help with that though.

@MilosRasic
Copy link
Contributor Author

Inactive. Closing.

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

No branches or pull requests

3 participants