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

Update invariant use within datastore generator functions #1941

Closed
aaemnnosttv opened this issue Aug 21, 2020 · 6 comments
Closed

Update invariant use within datastore generator functions #1941

aaemnnosttv opened this issue Aug 21, 2020 · 6 comments
Labels
P1 Medium priority QA: Eng Requires specialized QA by an engineer Type: Bug Something isn't working
Milestone

Comments

@aaemnnosttv
Copy link
Collaborator

aaemnnosttv commented Aug 21, 2020

Bug Description

In #1801 we updated the way createFetchStore works internally so that invalid use raises errors as expected.

As discovered in that issue, the main problem is that errors raised in a generator function do not throw but instead result in a rejected promise which is not handled well.

We still have many instances of invariant used within generators in the datastore outside of the createFetchStore use.

E.g.

*setModuleActivation( slug, active ) {
invariant( slug, 'slug is required.' );
invariant( active !== undefined, 'active is required.' );


Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

  • A new createValidatedAction utility should be created for simplifying the definition of an async action that has validation
    • It should take two arguments: validate (a non-generator function), actionCreator (a function which may or may not be a generator that returns and/or yields actions)
  • All instances of action creators in the data store which are generators that perform validation on parameters (i.e. invariant) should be updated to use the new utility

Implementation Brief

QA Brief

  • Review the codebase to make sure there are no longer any actions defined as generator functions which call invariant internally

Changelog entry

  • N/A
@aaemnnosttv aaemnnosttv added Type: Bug Something isn't working P1 Medium priority labels Aug 21, 2020
@aaemnnosttv aaemnnosttv self-assigned this Aug 21, 2020
@aaemnnosttv
Copy link
Collaborator Author

@felixarntz moving this forward to AC as there are still quite a few of these in the codebase.

@aaemnnosttv
Copy link
Collaborator Author

@felixarntz please review the ACs here as well. You can see what the proposed changes look like with one action here:
https://github.com/google/site-kit-wp/pull/3158/files?diff=split&w=1

@felixarntz
Copy link
Member

@aaemnnosttv I like the approach you're suggesting, great! IB ✅

@felixarntz felixarntz removed their assignment Apr 16, 2021
@fhollis fhollis added this to the Sprint 48 milestone Apr 21, 2021
@danielgent danielgent assigned danielgent and unassigned danielgent Apr 22, 2021
@eugene-manuilov eugene-manuilov removed their assignment Apr 27, 2021
@wpdarren
Copy link
Collaborator

@aaemnnosttv just checking this should be a QA:Eng ticket since it mentions looking at the codebase?

@aaemnnosttv aaemnnosttv added the QA: Eng Requires specialized QA by an engineer label Apr 28, 2021
@aaemnnosttv
Copy link
Collaborator Author

Yep, thanks @wpdarren – I've added it now 👍

@tofumatt tofumatt self-assigned this Apr 30, 2021
@tofumatt
Copy link
Collaborator

QA ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P1 Medium priority QA: Eng Requires specialized QA by an engineer Type: Bug Something isn't working
Projects
None yet
Development

No branches or pull requests

7 participants