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

Fix initial block settings data defaults, improve applySchemaDefaults and ObjectWidget schema support #3925

Merged
merged 26 commits into from Nov 19, 2022

Conversation

tiberiuichim
Copy link
Contributor

@tiberiuichim tiberiuichim commented Nov 18, 2022

I'll try to describe what this PR fixes:

We always had a problem with block default values extracted from schemas. The solution we had in place in InlineForm (based on repeat triggering onChangeField) is buggy because there wasn't a good way to avoid overriding the block value with old values. The reason is that onChangeField triggers a React state change. React batches multiple state changes, so in effect we're getting simultaneous calls like [setState({...oldState, newValue}), setState({...oldState, newValue})], and as you can see, the state is non-deterministic, it will overwrite itself with old values stored in a closure.

I've recently introduced a bug in the handling of default values, by making all fields like <Field value={props.value ?? props.default} />. This supperficially looks ok, because the inputs show up the correct default value. The problem is that this value is not propagated to the parent form state.

So the solution is to always make sure that, when we create a new "anything data" state based on a schema, we always try to apply the schema defaults on that data, before it's being passed to a callback handler and stored in a state. We do this in the BlocksForm on the onAddBlock and onInsertBlock (please look at this, maybe we need to do it in other block operations as well) and in the ObjectListWidget (which instantiates new objects based on schemas, when user clicks Add Item).

The ObjectWidget default value is another problem. If I look at a schema that has an object widget, I say "it's a regular field, you need to define the default prop for it, and that's it". But let's be more friendly, also support the existing code that uses default in objectwidget schemas. So we detect the object widget schema in applySchemaDefaults, and compute a value based on that. But then we have another issue: the ObjectWidget field value is a JS object ({}), but the ObjectListWidget also defines a schema prop, but its value is an Array ([]). So the ObjectListWidget schema prop should be renamed to itemSchema. For now I've added the condition in applySchemaDefaults.

To make sure we clean our act in InlineForm, we use onChangeFormData, which can mutate the whole form state, rather then one field at a time. In BlockDataform, we create the onChangeFormData by reusing the passed down onChangeBlock. So we get to the final thing, if you want properly working default values in your block, you should pass down onChangeBlock to your BlockDataForm

TODO:

  • console.warn if BlockDataForm is not passed down onChangeBlock
  • check if intl is passed to applySchemaDefaults, for BBB
  • more tests for applySchemaDefaults

@netlify
Copy link

netlify bot commented Nov 18, 2022

Deploy Preview for volto canceled.

Name Link
🔨 Latest commit 3273aec
🔍 Latest deploy log https://app.netlify.com/sites/volto/deploys/63792a3f80828b00092cea4c

@cypress
Copy link

cypress bot commented Nov 18, 2022



Test summary

444 0 20 0


Run details

Project Volto
Status Passed
Commit 3273aec
Started Nov 19, 2022 7:14 PM
Ended Nov 19, 2022 7:23 PM
Duration 09:35 💡
OS Linux Ubuntu -
Browser Chrome 107

View run in Cypress Dashboard ➡️


This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

@tiberiuichim
Copy link
Contributor Author

TODO: check if intl is passed to applySchemaDefaults, if it's not passed, don't try to compute schema defaults.

Also console.warn if BlockDataForm is not passed onChangeBlock.

@tiberiuichim
Copy link
Contributor Author

So the ObjectListWidget schema prop should be renamed to itemSchema

There's two proposal on that name:

  • itemSchema
  • objectSchema

My feeling, and the reason I think itemSchema is better, is because if we name it objectSchema, but the ObjectWidget keeps the schema prop, but it's still an "object schema", then why is it not named objectSchema as well? So, itemSchema to be different from "object schema", because the ObjectListWidget needs a different name for the schema.

@sneridagh sneridagh changed the base branch from master to 16.x.x November 19, 2022 18:53
sneridagh and others added 3 commits November 19, 2022 20:08
* 16.x.x:
  changelog
  Remove sentryOptions from settings reference Clean up deploying/sentry.md See #3922
@sneridagh sneridagh changed the title Init data with block defaults Fix initial block settings data defaults, improve applySchemaDefaults and ObjectWidget schema support Nov 19, 2022
@sneridagh sneridagh merged commit 7c3cf05 into 16.x.x Nov 19, 2022
@sneridagh sneridagh deleted the block_defaults branch November 19, 2022 19:28
sneridagh added a commit that referenced this pull request Nov 19, 2022
…s` and `ObjectWidget` schema support (#3925)

* Init data with block defaults

* Add start:coresandbox command; add some fields with default values

* WIP on cypress test

* Rest of test

* Add cy.wait

* Roll back changes to Field, no longer pass explicitely defaults; this is the job on the code that instantiates a new form, it needs to applySchemaDefaults

* Use onChangeFormData in InlineForm

* Pass down onChangeFormData from BlockDataForm

* Only pass down onChangeFormData if we have onChangeBlock

* Fix test block

* Fix defaults for ObjectListWidget

* Don't pass onChangeFormData

* Cleanup tests

* Remove comment

* Add ObjectWidget case

* Add cy.getContent; finish test for objectwidget / objectlistwidget defaults

* Code formatting

* Add changelog

* No need for async here

* One more test for the applySchemaDefaults helper

* Improve docs, add warning in upgrade guide

* Add console.warn notice in BlockDataForm if no onChangeBlock is passed down.

* More tests with different types of fields/widgets

* More resilient Cypress test

* BBB with applySchemaDefaults without intl passed down

Co-authored-by: Victor Fernandez de Alba <sneridagh@gmail.com>
sneridagh added a commit that referenced this pull request Nov 20, 2022
…s` and `ObjectWidget` schema support (#3925) (#3928)

* Init data with block defaults

* Add start:coresandbox command; add some fields with default values

* WIP on cypress test

* Rest of test

* Add cy.wait

* Roll back changes to Field, no longer pass explicitely defaults; this is the job on the code that instantiates a new form, it needs to applySchemaDefaults

* Use onChangeFormData in InlineForm

* Pass down onChangeFormData from BlockDataForm

* Only pass down onChangeFormData if we have onChangeBlock

* Fix test block

* Fix defaults for ObjectListWidget

* Don't pass onChangeFormData

* Cleanup tests

* Remove comment

* Add ObjectWidget case

* Add cy.getContent; finish test for objectwidget / objectlistwidget defaults

* Code formatting

* Add changelog

* No need for async here

* One more test for the applySchemaDefaults helper

* Improve docs, add warning in upgrade guide

* Add console.warn notice in BlockDataForm if no onChangeBlock is passed down.

* More tests with different types of fields/widgets

* More resilient Cypress test

* BBB with applySchemaDefaults without intl passed down

* Fix initial block settings data defaults, improve `applySchemaDefaults` and `ObjectWidget` schema support (#3925)

* Init data with block defaults

* Add start:coresandbox command; add some fields with default values

* WIP on cypress test

* Rest of test

* Add cy.wait

* Roll back changes to Field, no longer pass explicitely defaults; this is the job on the code that instantiates a new form, it needs to applySchemaDefaults

* Use onChangeFormData in InlineForm

* Pass down onChangeFormData from BlockDataForm

* Only pass down onChangeFormData if we have onChangeBlock

* Fix test block

* Fix defaults for ObjectListWidget

* Don't pass onChangeFormData

* Cleanup tests

* Remove comment

* Add ObjectWidget case

* Add cy.getContent; finish test for objectwidget / objectlistwidget defaults

* Code formatting

* Add changelog

* No need for async here

* One more test for the applySchemaDefaults helper

* Improve docs, add warning in upgrade guide

* Add console.warn notice in BlockDataForm if no onChangeBlock is passed down.

* More tests with different types of fields/widgets

* More resilient Cypress test

* BBB with applySchemaDefaults without intl passed down

Co-authored-by: Victor Fernandez de Alba <sneridagh@gmail.com>

* Fix initial block settings data defaults, improve `applySchemaDefaults` and `ObjectWidget` schema support (#3925)

* Init data with block defaults

* Add start:coresandbox command; add some fields with default values

* WIP on cypress test

* Rest of test

* Add cy.wait

* Roll back changes to Field, no longer pass explicitely defaults; this is the job on the code that instantiates a new form, it needs to applySchemaDefaults

* Use onChangeFormData in InlineForm

* Pass down onChangeFormData from BlockDataForm

* Only pass down onChangeFormData if we have onChangeBlock

* Fix test block

* Fix defaults for ObjectListWidget

* Don't pass onChangeFormData

* Cleanup tests

* Remove comment

* Add ObjectWidget case

* Add cy.getContent; finish test for objectwidget / objectlistwidget defaults

* Code formatting

* Add changelog

* No need for async here

* One more test for the applySchemaDefaults helper

* Improve docs, add warning in upgrade guide

* Add console.warn notice in BlockDataForm if no onChangeBlock is passed down.

* More tests with different types of fields/widgets

* More resilient Cypress test

* BBB with applySchemaDefaults without intl passed down

Co-authored-by: Victor Fernandez de Alba <sneridagh@gmail.com>

* Generator is aware of RC and beta versions when canary option is sele… (#3923)

* Generator is aware of RC and beta versions when canary option is selected

* Fix tests

* Add filled in data test

* Fix edge case when data is passed with values

Co-authored-by: Tiberiu Ichim <tiberiu.ichim@gmail.com>
Co-authored-by: Tiberiu Ichim <tiberiuichim@users.noreply.github.com>
sneridagh added a commit that referenced this pull request Nov 20, 2022
* master:
  Update documentation of Sentry integration. (#3927)
  Tidy up upgrade guide for v16.x.x (#3930) (#3932)
  Generator is aware of RC and beta versions when canary option is sele… (#3923) (#3929)
  Fix initial block settings data defaults, improve `applySchemaDefaults` and `ObjectWidget` schema support (#3925) (#3928)
  changelog
  Remove sentryOptions from settings reference Clean up deploying/sentry.md See #3922
@sneridagh sneridagh mentioned this pull request Nov 29, 2022
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.

None yet

2 participants