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

Use preloading for initial requests in importer and setup wizard #3446

Merged
merged 10 commits into from
Aug 6, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 5 additions & 11 deletions assets/data-port/import.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,30 +3,28 @@ import { useSelect, useDispatch } from '@wordpress/data';
import { render, useLayoutEffect } from '@wordpress/element';
import { DataPortStepper } from './stepper';
import registerImportStore from './import/data';
import { Spinner } from '@woocommerce/components';
import { Notice } from '@wordpress/components';
import '../shared/data/api-fetch-preloaded-once';

registerImportStore();

/**
* Sensei import page.
*/
const SenseiImportPage = () => {
const { isFetching, error, navigationSteps } = useSelect( ( select ) => {
const { error, navigationSteps } = useSelect( ( select ) => {
renatho marked this conversation as resolved.
Show resolved Hide resolved
const store = select( 'sensei/import' );
return {
isFetching: store.isFetching(),
error: store.getFetchError(),
navigationSteps: store.getNavigationSteps(),
};
}, [] );

const { fetchCurrentJobState } = useDispatch( 'sensei/import' );
const { loadCurrentJobState } = useDispatch( 'sensei/import' );

// We want to show the loading before any content.
useLayoutEffect( () => {
fetchCurrentJobState();
}, [ fetchCurrentJobState ] );
loadCurrentJobState();
}, [ loadCurrentJobState ] );

// Add `sensei-color` to body tag.
useLayoutEffect( () => {
Expand All @@ -35,10 +33,6 @@ const SenseiImportPage = () => {
return () => document.body.classList.remove( [ 'sensei-color' ] );
} );

if ( isFetching ) {
return <Spinner className="sensei-import__main-loader" />;
}

if ( error ) {
return (
<Notice status="error" isDismissible={ false }>
Expand Down
16 changes: 8 additions & 8 deletions assets/data-port/import/data/actions.js
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
import {
API_SPECIAL_ACTIVE_JOB_ID,
FETCH_FROM_API,
START_FETCH_CURRENT_JOB_STATE,
SUCCESS_FETCH_CURRENT_JOB_STATE,
ERROR_FETCH_CURRENT_JOB_STATE,
START_LOAD_CURRENT_JOB_STATE,
SUCCESS_LOAD_CURRENT_JOB_STATE,
ERROR_LOAD_CURRENT_JOB_STATE,
START_IMPORT,
SUCCESS_START_IMPORT,
ERROR_START_IMPORT,
Expand Down Expand Up @@ -47,17 +47,17 @@ export const wait = ( timeout ) => ( {
/**
* Fetch importer data for current job.
*/
export const fetchCurrentJobState = composeFetchAction(
START_FETCH_CURRENT_JOB_STATE,
export const loadCurrentJobState = composeFetchAction(
START_LOAD_CURRENT_JOB_STATE,
function* () {
const data = yield fetchFromAPI( {
path: buildJobEndpointUrl( API_SPECIAL_ACTIVE_JOB_ID ),
} );

return normalizeImportData( data );
},
SUCCESS_FETCH_CURRENT_JOB_STATE,
ERROR_FETCH_CURRENT_JOB_STATE
SUCCESS_LOAD_CURRENT_JOB_STATE,
ERROR_LOAD_CURRENT_JOB_STATE
);

/**
Expand Down Expand Up @@ -416,5 +416,5 @@ export const resetState = () => ( {
*/
export function* restartImporter() {
yield resetState();
yield fetchCurrentJobState();
yield loadCurrentJobState();
}
18 changes: 9 additions & 9 deletions assets/data-port/import/data/actions.test.js
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
import {
API_BASE_PATH,
FETCH_FROM_API,
START_FETCH_CURRENT_JOB_STATE,
SUCCESS_FETCH_CURRENT_JOB_STATE,
ERROR_FETCH_CURRENT_JOB_STATE,
START_LOAD_CURRENT_JOB_STATE,
SUCCESS_LOAD_CURRENT_JOB_STATE,
ERROR_LOAD_CURRENT_JOB_STATE,
START_IMPORT,
SUCCESS_START_IMPORT,
ERROR_START_IMPORT,
Expand All @@ -17,7 +17,7 @@ import {

import {
fetchFromAPI,
fetchCurrentJobState,
loadCurrentJobState,
submitStartImport,
startImport,
successStartImport,
Expand Down Expand Up @@ -102,11 +102,11 @@ describe( 'Importer actions', () => {
* Fetch importer data action.
*/
it( 'Should generate the get current job state importer data action', () => {
const gen = fetchCurrentJobState();
const gen = loadCurrentJobState();

// Start fetch action.
const expectedStartFetchAction = {
type: START_FETCH_CURRENT_JOB_STATE,
type: START_LOAD_CURRENT_JOB_STATE,
};

expect( gen.next().value ).toEqual( expectedStartFetchAction );
Expand Down Expand Up @@ -142,7 +142,7 @@ describe( 'Importer actions', () => {
},
},
},
type: SUCCESS_FETCH_CURRENT_JOB_STATE,
type: SUCCESS_LOAD_CURRENT_JOB_STATE,
};

expect( gen.next( RESPONSE_FULL ).value ).toEqual(
Expand All @@ -151,7 +151,7 @@ describe( 'Importer actions', () => {
} );

it( 'Should catch error on the get current job state action', () => {
const gen = fetchCurrentJobState();
const gen = loadCurrentJobState();

// Start fetch action.
gen.next();
Expand All @@ -162,7 +162,7 @@ describe( 'Importer actions', () => {
// Error action.
const error = { code: '', message: 'Error' };
const expectedErrorAction = {
type: ERROR_FETCH_CURRENT_JOB_STATE,
type: ERROR_LOAD_CURRENT_JOB_STATE,
error,
};
expect( gen.throw( error ).value ).toEqual( expectedErrorAction );
Expand Down
9 changes: 4 additions & 5 deletions assets/data-port/import/data/constants.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,11 @@ export const FETCH_FROM_API = 'FETCH_FROM_API';
export const WAIT = 'WAIT';

/**
* Fetch action type constants.
* Load action type constants.
*/
export const START_FETCH_CURRENT_JOB_STATE = 'START_FETCH_CURRENT_JOB_STATE';
export const SUCCESS_FETCH_CURRENT_JOB_STATE =
'SUCCESS_FETCH_CURRENT_JOB_STATE';
export const ERROR_FETCH_CURRENT_JOB_STATE = 'ERROR_FETCH_CURRENT_JOB_STATE';
export const START_LOAD_CURRENT_JOB_STATE = 'START_LOAD_CURRENT_JOB_STATE';
export const SUCCESS_LOAD_CURRENT_JOB_STATE = 'SUCCESS_LOAD_CURRENT_JOB_STATE';
export const ERROR_LOAD_CURRENT_JOB_STATE = 'ERROR_LOAD_CURRENT_JOB_STATE';
export const SET_JOB_STATE = 'SET_JOB_STATE';

/**
Expand Down
4 changes: 2 additions & 2 deletions assets/data-port/import/data/normalizer.js
Original file line number Diff line number Diff line change
Expand Up @@ -60,8 +60,8 @@ export const normalizeImportData = ( {
...data,
jobId: id,
progress: status,
upload: normalizeUploadsState( files ),
completedSteps: parseCompletedSteps( status ),
upload: normalizeUploadsState( files || [] ),
completedSteps: parseCompletedSteps( status || {} ),
done: {
results,
},
Expand Down
18 changes: 7 additions & 11 deletions assets/data-port/import/data/reducer.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import {
START_FETCH_CURRENT_JOB_STATE,
SUCCESS_FETCH_CURRENT_JOB_STATE,
ERROR_FETCH_CURRENT_JOB_STATE,
START_LOAD_CURRENT_JOB_STATE,
SUCCESS_LOAD_CURRENT_JOB_STATE,
ERROR_LOAD_CURRENT_JOB_STATE,
START_IMPORT,
SUCCESS_START_IMPORT,
ERROR_START_IMPORT,
Expand Down Expand Up @@ -87,14 +87,14 @@ const updateLevelState = ( state, levelKey, attributes ) => ( {
*/
export default ( state = DEFAULT_STATE, action ) => {
switch ( action.type ) {
case START_FETCH_CURRENT_JOB_STATE:
case START_LOAD_CURRENT_JOB_STATE:
return {
...state,
isFetching: true,
fetchError: false,
};

case SUCCESS_FETCH_CURRENT_JOB_STATE:
case SUCCESS_LOAD_CURRENT_JOB_STATE:
return {
...merge( {}, state, action.data ),
isFetching: false,
Expand All @@ -105,15 +105,11 @@ export default ( state = DEFAULT_STATE, action ) => {
...merge( {}, state, action.data ),
};

case ERROR_FETCH_CURRENT_JOB_STATE:
// No need to start a new job until we have our first active upload.
const isErrorNoActiveJob =
action.error.code === 'sensei_data_port_job_not_found';

case ERROR_LOAD_CURRENT_JOB_STATE:
return {
...state,
isFetching: false,
fetchError: isErrorNoActiveJob ? false : action.error,
fetchError: action.error,
};

case START_IMPORT:
Expand Down
12 changes: 6 additions & 6 deletions assets/data-port/import/data/reducer.test.js
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
import reducer from './reducer';
import {
START_FETCH_CURRENT_JOB_STATE,
SUCCESS_FETCH_CURRENT_JOB_STATE,
ERROR_FETCH_CURRENT_JOB_STATE,
START_LOAD_CURRENT_JOB_STATE,
SUCCESS_LOAD_CURRENT_JOB_STATE,
ERROR_LOAD_CURRENT_JOB_STATE,
START_IMPORT,
SUCCESS_START_IMPORT,
ERROR_START_IMPORT,
Expand All @@ -20,7 +20,7 @@ import {
describe( 'Importer reducer', () => {
it( 'Should set isFetching to true on START_FETCH_CURRENT_JOB_STATE action', () => {
const state = reducer( undefined, {
type: START_FETCH_CURRENT_JOB_STATE,
type: START_LOAD_CURRENT_JOB_STATE,
} );

expect( state.isFetching ).toBeTruthy();
Expand All @@ -32,7 +32,7 @@ describe( 'Importer reducer', () => {
};

const state = reducer( undefined, {
type: SUCCESS_FETCH_CURRENT_JOB_STATE,
type: SUCCESS_LOAD_CURRENT_JOB_STATE,
data,
} );

Expand Down Expand Up @@ -60,7 +60,7 @@ describe( 'Importer reducer', () => {
};

const state = reducer( undefined, {
type: ERROR_FETCH_CURRENT_JOB_STATE,
type: ERROR_LOAD_CURRENT_JOB_STATE,
error,
} );

Expand Down
2 changes: 1 addition & 1 deletion assets/data-port/import/helpers/url.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import { API_BASE_PATH } from '../data/constants';
* @param {Array} parts Parts of the URL.
* @return {string} Combined URL.
*/
export const buildJobEndpointUrl = ( jobId, parts ) => {
export const buildJobEndpointUrl = ( jobId, parts = null ) => {
const path = parts ? '/' + parts.join( '/' ) : '';

return API_BASE_PATH + jobId + path;
Expand Down
2 changes: 1 addition & 1 deletion assets/setup-wizard/data/actions.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ export function* fetchSetupWizardData() {

try {
const data = yield fetchFromAPI( {
path: API_BASE_PATH,
path: API_BASE_PATH.replace( /\/$/, '' ),
} );
yield successFetch( normalizeSetupWizardData( data ) );
} catch ( error ) {
Expand Down
2 changes: 1 addition & 1 deletion assets/setup-wizard/data/actions.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ describe( 'Setup wizard actions', () => {
const expectedFetchAction = {
type: FETCH_FROM_API,
request: {
path: API_BASE_PATH,
path: '/sensei-internal/v1/setup-wizard',
},
};
expect( gen.next().value ).toEqual( expectedFetchAction );
Expand Down
2 changes: 1 addition & 1 deletion assets/setup-wizard/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { render, useLayoutEffect } from '@wordpress/element';
import { Spinner } from '@woocommerce/components';
import { Notice } from '@wordpress/components';
import { __ } from '@wordpress/i18n';

import '../shared/data/api-fetch-preloaded-once';
import registerSetupWizardStore from './data';
import { useWpAdminFullscreen } from '../react-hooks';

Expand Down
28 changes: 28 additions & 0 deletions assets/shared/data/api-fetch-preloaded-once.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
import { addQueryArgs } from '@wordpress/url';
import apiFetch from '@wordpress/api-fetch';

/**
* Use data preloaded with createPreloadingMiddleware only once.
*/
export function preloadedDataUsedOnceMiddleware() {
const usedPreload = {};

return ( request, next ) => {
if (
'string' === typeof request.path &&
( 'GET' === request.method || ! request.method )
) {
if ( usedPreload[ request.path ] ) {
request.path = addQueryArgs( request.path, {
__skip_preload: 1,
} );
} else {
usedPreload[ request.path ] = true;
}
}

return next( request );
};
}

apiFetch.use( preloadedDataUsedOnceMiddleware() );
35 changes: 35 additions & 0 deletions assets/shared/data/api-fetch-preloaded-once.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
import apiFetch from '@wordpress/api-fetch';
import { preloadedDataUsedOnceMiddleware } from './api-fetch-preloaded-once';

describe( 'preloadedDataUsedOnceMiddleware', () => {
beforeEach( () => {
apiFetch.use(
apiFetch.createPreloadingMiddleware( {
'/test': { body: 'preloaded-result' },
} )
);
apiFetch.use( preloadedDataUsedOnceMiddleware() );
window.fetch = jest.fn();
} );

it( 'loads cached data on the first request', async () => {
const result = await apiFetch( {
path: '/test',
} );

expect( result ).toEqual( 'preloaded-result' );
} );

it( 'loads fresh data on additional requests', async () => {
window.fetch.mockReturnValue( Promise.reject( null ) );
await apiFetch( { path: '/test' } );
try {
await apiFetch( { path: '/test' } );
} catch ( err ) {}

expect( window.fetch ).toHaveBeenCalledWith(
expect.stringContaining( '/test' ),
expect.anything()
);
} );
} );
1 change: 1 addition & 0 deletions includes/admin/class-sensei-setup-wizard.php
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,7 @@ public function enqueue_scripts() {
Sensei()->assets->wp_compat();
Sensei()->assets->enqueue( 'sensei-setup-wizard', 'setup-wizard/index.js', [ 'sensei-event-logging', 'wp-i18n' ], true );
$this->setup_wizard_set_script_translations();
Sensei()->assets->preload_data( [ '/sensei-internal/v1/setup-wizard' ] );
}

/**
Expand Down
1 change: 1 addition & 0 deletions includes/data-port/class-sensei-import.php
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ function() {
Sensei()->assets->wp_compat();
Sensei()->assets->enqueue( 'sensei-import', 'data-port/import.js', [], true );
wp_set_script_translations( 'sensei-import', 'sensei-lms' );
Sensei()->assets->preload_data( [ '/sensei-internal/v1/import/active' ] );
}
);

Expand Down
Loading