introduce middleware support #2220
Conversation
✅ Deploy Preview for jsonforms-examples ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
dholmes2409
changed the title
| @@ -1100,3 +1101,49 @@ test('JsonForms should use react to additionalErrors update', () => { | |||
| expect(wrapper.find('h5').text()).toBe('Foobar'); | |||
| wrapper.unmount(); | |||
| }); | |||
|
|
|||
| test('JsonForms should used provided middleware if available', () => { | |||
There was a problem hiding this comment.
@sdirix if there are any other scenarios you think would be good to add let me know, this was the most immediate one I could think of
There was a problem hiding this comment.
Hi! That's already great!
What's tested now is that if the middleware rejects a modification, that then onChangeHandler was not called. We should also check that the rendered value did not change.
It would be good to also test the modification case. For example if the middleware modifies an additional value in addition to the regular update and returns that as the new state. Then the onChange handler should also be given that modified value and the new value should be rendered.
I think then we have all basic cases covered!
There was a problem hiding this comment.
I've added a test case for modifying the state value within the middleware, hopefully this covers the modification case. I wasn't fully sure what you meant by Then the onChange handler should also be given that modified value and the new value should be rendered.. Wouldn't the onChange handler only be called if we explicitly call it within the middleware?
There was a problem hiding this comment.
Hi @sdirix 👋, how goes it? Just a reminder that the extra unit test you asked about has been added, and a query as to whether anything else will be needed before this PR can be further progressed?
|
|
||
| // then | ||
| expect(customMiddleware).toHaveBeenCalledTimes(1); | ||
| expect(onChangeHandler).not.toHaveBeenCalled(); |
There was a problem hiding this comment.
To test the onChangeHandler we have to use a timeout, like in the other tests.
Actually the onChangeHandler should have been called 1 times here because the initial rendering of JsonForms will trigger one emit. Then the update afterwards should not trigger one as we do return state in the controlledMiddleware, therefore the state will not be updated and there should not be another emit.
|
|
||
| // then | ||
| expect(customMiddleware).toHaveBeenCalledTimes(1); | ||
| expect(onChangeHandler).not.toHaveBeenCalled(); |
There was a problem hiding this comment.
Same here, in fact the onChangeHandler should have been called 2 times after waiting a bit. The first time because of the initial rendering. The second time because we allow the state update to go through, which should lead to an additional call of onChangeHandler.
There was a problem hiding this comment.
I've added those checks now, let me know if the tests are how you were expecting them
This update introduces optional middleware support for our React bindings, enabling advanced features like controlled mode in JSON Forms and on-the-fly handling of derived attributes.
|
@dchambers @dholmes2409 Thank you so much for all of the contribution work ❤️ and architecture discussions. Without your help this feature would not arrived so soon. |
No, thank you helping us land this @sdirix 🙇 |
Resolves #2174
Introduces middleware support which can override state actions 'such as UPDATE' allowing for handling of controlled states effectively bypassing the debounce