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

Improve tx page state management #866

Merged
merged 3 commits into from
Jul 11, 2018
Merged

Improve tx page state management #866

merged 3 commits into from
Jul 11, 2018

Conversation

lyyder
Copy link
Contributor

@lyyder lyyder commented Jul 10, 2018

The state management in the loadData function in TransactionPage.duck has been somewhat problematic and this PR aims to improve it.

Now loadData checks if a transactionRef is found from state. It is set when a tx fetch succeeds. If a transactionRef is found it means the state holds data from a previous page load and the state is cleared. If the ref can not be found it means that the page has not been loaded or that the setInitialValues has been invoked for example with an initial message sending error information from CheckoutPage.

@lyyder lyyder requested a review from Gnito July 10, 2018 13:01
// In case a transaction reference is found from a previous
// data load -> clear the state. Otherwise keep the non-null
// values which may have been set from a previous page.
const initialValues = txRef ? {} : pickBy(state, v => !!v);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Arrays are a problem here.
!![] => true

v => (Array.isArray(v) && !!v.length) || !!v

Copy link
Contributor Author

@lyyder lyyder Jul 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Gnito I thought about that and reasoned that if the state has en empty array it can be passed as an initial value as in initialState the value of messages, which seems to be the only array, is [].

However, what you proposed would be more explicit. Also empty objects are truthy too and for example there's the transactionRef which is an object and should be overridden with the value from initialState in case it is somehow has been set to {}.

I'll add handling for empty arrays and objects.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, passing [] to

(Array.isArray(v) && !!v.length) || !!v

resolves to true

Copy link
Contributor

@Gnito Gnito left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check the comment

Also override empty arrays and objects with values from initialState.
@lyyder lyyder merged commit e5128b7 into master Jul 11, 2018
@lyyder lyyder deleted the tx-page-state branch July 11, 2018 09:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants