Options: Add data store

[psealock]

Adds an options data store to replace wc-api/options.

Testing Instructions

1. Smoke test the OBW and Task List.
2. Check for no differences in behaviour.
3. The "Customize your store" task list and payment steps included the most significant refactors of logic.

[becdetat]

Could you do this with an if statement? It seems a little complicated for what it does.

[psealock]

I originally had it like this:

withOptionsHydration( {
    ...( window.wcSettings.preloadOptions || {} ),
} )

It was nicer to read, but I'd rather not call the hydration function if the data isn't there because resolvers in the data store will cache the empty values, preventing future api requests from going through.

[becdetat]

You've used 'recieved' instead of 'received'. Apart from that and the question about an if this looks good :-)

[joshuatf]

Will give this a more thorough look and test on Monday, but overall looks really solid. 💯

Left a couple comments and possible update to how we check for requesting. Thanks for all your effort here, Paul! This is really awesome work.

[joshuatf]

I think we can replace these additional selectors with those built in to wp data. Outlined an issue with more detail here.

[psealock]

Thats an excellent idea! The way this is setup is getOptions calls resolver getOptionsWithRequest if the option hasn't been preloaded, which leaves us with this:

const options = getOptions( args );
const isRequesting = ( 'getOptionsWithRequest', [ args ] ) );
// What we really want is this:
// const isRequesting = ( 'getOptions', [ args ] ) );

More on why its that way in p90Yrv-1uN#comment-3842.

One option is to introduce isOptionsResolving to map resolver names. This is less than ideal because I'd rather hide implementation details from the consumer.

export const isOptionsResolving = ( state, selectorName, args ) => {
	return select( STORE_NAME ).isResolving(
		selectorName === 'getOptions' ? 'getOptionsWithRequest' : selectorName,
		args
	);
};

Any other ideas?

[joshuatf]

Another option would be to get rid of getOptions all together and just have any request for data always go through getOption to simplify the code.

I think I like this idea the best. It's a simple and easy to understand API and fixes issues with caching or needing to call with the exact same arguments used to determine resolution.

I originally wrote getOptions with an array for the purpose of avoiding multiple trips to the server when multiple options were needed, but if we wanted to keep this from a performance perspective, maybe we could collect requests for options and then fetch at the end of the stack.

For example, assume option A is cached but B and C need to be fetched and optionsToRequest and fetches are available across calls.

// in component
const a = getOption( 'a' );
const b = getOption( 'b' );
const c = getOption( 'c' );

// in resolvers, store a persisted array of items that need to be requested
optionsToRequest.push( optionName );

// Use a timeout of 1ms to push to the end of the stack
setTimeout( () => {
  // optionsToRequest should now be ['b', 'c']
  // Avoid fetching multiple times for a group of options.
  fetchPromise = fetches[ optionsToRequest.join(',') ]
  if ( fetchStarted ) {
     yield fetchPromise;
     return;
  }

  // fetch options in array optionsToRequest
  fetches[ optionsToRequest.join(',') ]  = yield apiFetch( {
    path: url,
    method: 'GET',
    data: optionsToRequest
   } );
  
}, 1 );

Do you think something like that is feasible?

[psealock]

Nice one @joshuatf !

I like the request collection technique. The thing that deterred me from this solution was it complicating the hydration part because I'll need to loop through all of the options individually. I got a solution to that one though.

[timmyc]

@psealock looks like this might need a rebase ( and possibly a tough one at that ) - is there more work that needs to be done here, and can we coordinate it in such a way so you don't get hit with another hefty merge conflict?

[psealock]

@timmyc or @joshuatf Can you give this a look?

[timmyc]

@psealock thanks for the work here and keeping on moving us forward on to wp.data.

Onboarding Profiler worked great for me. I'm seeing one oddity on the Setup Checklist. After configuring manual payment types ( COD and BACS ) the Payment step still was not being marked as done in the task list. I setup PayPal, and still, the option is never marked as completed. I don't think I have hit this before, so wondering if it is due to the changes here. I did not test in master to verify:

[psealock]

I'm seeing one oddity on the Setup Checklist.

Yup, thanks. Thats an error which I fixed in 0a7ce3d.

I also rebased against master.

[joshuatf]

Testing really well for me. Awesome work, Paul!

Added one nitpick around statuses, but pre-approving.

[joshuatf]

One minor nitpick, but I think we could update these to success: true|false to make checking easier and not manually checking against a string each time.

Though I think moving to an error-only checking approach may be more lean long-term (including response changes I recently made in the REST APIs). See p90Yrv-1KL-p2#comment-3981 for discussion around this.

[joshuatf]

😆

[psealock]

Thanks for the review @joshuatf. I fixed the status to success: true | false in db3eed0