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

Jump to bottom

LPS-129529 Adds the Data representation implementation of Forms to Data Engine #100493

Closed
wants to merge 1 commit into from
Closed

LPS-129529 Adds the Data representation implementation of Forms to Data Engine #100493

wants to merge 1 commit into from

Conversation

liferay-continuous-integration
Copy link
Collaborator

Forwarded from: liferay-forms#167 (Took 1 ci:forward attempt in 58 seconds)
Console

@matuzalemsteles
@liferay-forms

Original pull request comment:
To understand more about the reason for this implementation read the ticket: https://issues.liferay.com/browse/LPS-129529

The useDataView hook is the component that ends up doing the entire orchestration of creating the representation of the data, reusing, revalidating, caching, and returning the output of the representation.

Key ideas

Some resources make it very valuable and significant for the growth of the application and how it makes it easier for two different structures to coexist but with the same raw data.

  • The components never know that these two different structures exist, because there is no need to call any method to convert the data.
  • It is easy to slowly migrate components to use the new data structure.
  • It will be simpler to maintain data representations based on the implementation of "schema" compared to data conversion.
  • We don't couple any method or class to the components that we need to call for every corner, like DataLayoutBuilder that was done before.
  • It is not necessary to wait to convert all the data before starting to render the component or to call any other method.
  • We create data representation whenever a possible using reference to avoid overloading the creation of new objects being allocated in the heap.
  • When a Schema is created for the first time it is reused so we don't need to create another one again, there is a reuse of schema in different instances of the components if you use the same representation.
  • There is a revalidation of the raw data to avoid loss of reference.
  • For nested data such as dataLayout.dataLayoutPages there is a great benefit for the implementation of getter, it will be invoked only when it is used, if you only need to get only the title of the page the representation/conversion in the structure of dataLayoutRows will not be done, so we have the benefit of creating the lazy representation here.

Despite having a lot of good things with this implementation I would say that there is no contraindication just that it adds minimal overhead to the data output which is incredibly small in the 100ns range but that this is a great pros due to the previous implementation that did a lot data conversion all the time which was considerably noticeable at times the slowness.

Design decision

The useDataView hook is not used directly, it is built into useFormState to avoid coupling its use to the components. The logic is not done on top of useFormState because it is based on principles of responsibility, this helps when all components are using the Data Engine data structure, the swap will be easy to do.

Components that already consume useFormState, can already benefit from this.

const {dataLayout} = useFormState({schema: ['dataLayout']});

There may be cases where components consume the data in the Forms and Data Engine structure, although this is not a good practice you can instantiate two useFormState to consume the data in a different way or if the component needs information that does not compare.

const {focusedField} = useFormState();
const {dataLayout} = useFormState({schema: ['dataLayout']});

Drawbacks

  • Having to define which data will be used, avoids having to create all schemes at once.
  • When the Schema is defined, only the schema is returned, no merge is made with the rest of the store, in case it is necessary to instantiate a new useFormState to get the data.
  • Undeclared data within the Schema are not exposed at the exit, just expose the methods that are used.

Notes

  • This implementation is not yet complete, it does not cover the case of creating a reducer using the state represented in the Data Engine structure. Adding in next interactions.
  • Also, not all data representations were added, just adding the representation for DataLayout and DataDefinition initially. Add more methods or more Schemas as needed.

✔️ ci:test:stable - 9 out of 9 jobs passed

✔️ ci:test:relevant - 26 out of 26 jobs passed in 2 hours 52 minutes

Click here for more details.

Base Branch:

Branch Name: master
Branch GIT ID: 542ac19ddfbb24263d429cb2041b65f6d8c42f73

Upstream Comparison:

Branch GIT ID: fdde6ad10f71812222a8c02546fa909e3fcc295a
Jenkins Build URL: Acceptance Upstream DXP (master) #1706

ci:test:stable - 9 out of 9 jobs PASSED
9 Successful Jobs:
ci:test:relevant - 26 out of 26 jobs PASSED
26 Successful Jobs:
For more details click here.

✔️ ci:test:sf - 1 out of 1 jobs passed in 4 minutes

Click here for more details.

Base Branch:

Branch Name: master
Branch GIT ID: de28124dfde97c9a709ce440ca1d3965d42fa0f0

Sender Branch:

Branch Name: LPS-129529
Branch GIT ID: 0fec386ae193825a9bdbae957ebd59e42539f8ae

1 out of 1jobs PASSED
1 Successful Jobs:
For more details click here.

@matuzalemsteles
LPS-129529 Adds the Data representation implementation of Forms to Da…
0fec386 
…ta Engine

The `useDataView` hook is the component that ends up doing the entire orchestration of creating the representation of the data, reusing, revalidating, caching, and returning the output of the representation.

Some resources make it very valuable and significant for the growth of the application and how it makes it easier for two different structures to coexist but with the same raw data.

- The components never know that these two different structures exist, because there is no need to call any method to convert the data.
- It is easy to slowly migrate components to use the new data structure.
- It will be simpler to maintain data representations based on the implementation of "schema" compared to data conversion.
- We don't couple any method or class to the components that we need to call for every corner, like DataLayoutBuilder that was done before.
- It is not necessary to wait to convert all the data before starting to render the component or to call any other method.
- We create data representation whenever possible using reference to avoid overloading the creation of new objects being allocated in the heap.
- When a Schema is created for the first time it is reused so we don't need to create another one again, there is a reuse of schema in different instances of the components if you use the same representation.
- There is a revalidation of the raw data to avoid loss of reference.

Despite having a lot of good things with this implementation I would say that there is no contraindication just that it adds minimal overhead to the data output which is incredibly small in the 100ns range but that this is a great pros due to the previous implementation that did a lot data conversion all the time which was considerably noticeable at times the slowness.

The `useDataView` hook is not used directly, it is built into `useFormState` to avoid coupling its use to the components. The logic is not done on top of `useFormState` because it is based on principles of responsibility, this helps when all components are using the Data Engine data structure, the swap will be easy to do.

This implementation is not yet complete, it does not cover the case of creating a reducer using the state represented in the Data Engine structure. Adding in next interactions.

Also, not all data representations were added, just adding the representation for DataLayout and DataDefinition initially. Add more methods or more Schemas as needed.
@liferay-continuous-integration liferay-continuous-integration mentioned this pull request Apr 1, 2021
Closed
@liferay-continuous-integration
Copy link
Collaborator Author

To conserve resources, the PR Tester does not automatically run for forwarded pull requests.

@brianchandotcom
Copy link
Owner

Well done @matuzalemsteles

@brianchandotcom
Copy link
Owner

Merged. Thank you.
View total diff: a877e28...068a58e

@liferay-continuous-integration liferay-continuous-integration deleted the ci-forward-LPS-129529-pr-167-sender-matuzalemsteles-ts-1617288811421 branch April 9, 2021 10:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
ci:test:relevant - success ci:test:sf - success ci:test:stable - success
Projects
None yet
Milestone
No milestone
3 participants
@liferay-continuous-integration @brianchandotcom @matuzalemsteles