Feature/new context api #198
Conversation
src/index.tsx
Outdated
| this.props.children, | ||
| return ( | ||
| <FormsyContext.Provider value={this.contextValue}> | ||
| {React.createElement( |
There was a problem hiding this comment.
Any reason to not convert this to jsx? Mixing jsx and React.createElement seems messy
src/index.tsx
Outdated
| this.setState({ | ||
| canChange: true, | ||
| }); | ||
| onValidationComplete(); |
There was a problem hiding this comment.
Oh, and i also fixed a bug, where if you remove all the inputs but the last one was invalid, the form was still invalid. The callback onValidationComplete was never called.
|
Is this something we should have prepped but hold back on due to incompatibility? Enzyme is pretty serious, and I'd hate to make 2.x so bleeding edge nobody can use it. We're going to have a hard enough time with the release. :/ |
|
I totally forgot about this PR, I will try to fix it this week. |
|
Great! I'm going to release 2.0.0, but I expect a 2.0.1 fix-release will follow pretty quickly, and it would be nice to add this. |
| @@ -497,6 +502,7 @@ class Formsy extends React.Component<FormsyProps, FormsyState> { | |||
| this.setState({ | |||
| canChange: true, | |||
| }); | |||
| onValidationComplete(); | |||
There was a problem hiding this comment.
Is this related to context, or is it an unrelated bug-fix, or an accident?
There was a problem hiding this comment.
#198 (comment)
Yes, when the last input is removed the form was still invalid (if the removed input was invalid)
src/index.ts
Outdated
| attachToForm: this.attachToForm, | ||
| detachFromForm: this.detachFromForm, | ||
| isFormDisabled: props.disabled, | ||
| isValidValue: (component, value) => this.runValidation(component, value).isValid, |
There was a problem hiding this comment.
Why was this method deleted and put into the context like this?
| @@ -150,27 +152,23 @@ export default function<T, V>( | |||
| this.setValidations(validations, required); | |||
|
|
|||
| // Pass a function instead? | |||
| formsy.attachToForm(this); | |||
| attachToForm(this); | |||
There was a problem hiding this comment.
Is it considered better to have your context not be wrapped in a single namespace like formsy as it was before?
There was a problem hiding this comment.
The way we subscribe the the context is with the static property ContextType. Subscribe to multiple contexts with a static property are deprecated.
| const noFormsyErrorMessage = 'Could not find Formsy Context Provider. Did you use withFormsy outside Formsy ?'; | ||
|
|
||
| const throwNoFormsyProvider = () => { | ||
| throw new Error(noFormsyErrorMessage); |
There was a problem hiding this comment.
Will this stop formsy components from being able to be used outside of Formsy without throwing errors?
This may be too harsh a response. That said, I do like that it's very clear to users who are making a mistake and I normally really appreciate helpful errors like this.
I guess you could always export your input components twice, once withFormsy and once without.
There was a problem hiding this comment.
Yes it will crash the application if not catched. The same behavior as react-redux if you call connect outside a Provider.
If you think it should only print a error in the console let me know.
| "react": "^15.6.1 || ^16.0.0", | ||
| "react-dom": "^15.6.1 || ^16.0.0" | ||
| "react": "^16.0.0", | ||
| "react-dom": "^16.0.0" |
There was a problem hiding this comment.
It's been 2 years, so this seems like an acceptable change.
src/Wrapper.ts
Outdated
| @@ -232,7 +228,7 @@ export default function<T, V>( | |||
| isPristine: false, | |||
| }, | |||
| () => { | |||
| formsy.validate(this); | |||
| this.context.validate(this); //eslint-disable-line | |||
There was a problem hiding this comment.
Can we add the rule here? It would be nice to know what eslint is mad about.
src/index.ts
Outdated
| @@ -55,6 +57,7 @@ export interface FormsyState { | |||
| isPristine?: boolean; | |||
| isSubmitting: boolean; | |||
| isValid: boolean; | |||
| contextValue: FormsyContextInterface; | |||
| } | |||
There was a problem hiding this comment.
This should be alphabetically sorted.
|
@ThibautMarechal This is looking great! Left comments. They are mostly questions and very light suggestions, so feel free to respond to any comments if you prefer the way you did it. A lot of it was just questions I had about context and best practices since I'm not familiar with that part of React. One additional change is that you can drop all the dist changes. Jest runs directly on the source code and all dist changes are computed and committed automatically when I run the deploy script. It makes PRs so much easier to read and reduces conflicts in people's PRs. |
Co-Authored-By: Robert Kuykendall <[email protected]>
Co-Authored-By: Robert Kuykendall <[email protected]>
…arechal/formsy-react into feature/new-context-api
|
Very strange that the build files are showing as changed even though they look exactly the same. Maybe you're on windows and it changed the line endings? I'm okay to just merge it if we can't figure out why those are still showing as changed. |
Co-Authored-By: Robert Kuykendall <[email protected]>
For now Formsy use the legacy context api. (https://reactjs.org/docs/legacy-context.html)
We should use the new context api. (https://reactjs.org/docs/context.html)
So we don't have warnings when running in strict-mode.
I may need some help to fix the tests issue :/
It seems that the context isn't fully supported by Enzyme ? (enzymejs/enzyme#1553)
Or I miss something.