search

google / site-kit-wp

Site Kit is a one-stop solution for WordPress users to use everything Google has to offer to make them successful on the web.
https://sitekit.withgoogle.com
Apache License 2.0
1.25k stars 291 forks source link

Remove instances of "return yield" in the codebase #9118

Open techanvil opened 3 months ago

techanvil commented 3 months ago

Feature Description

We have a number of cases where we're currently returning the result of a yield in an action generator. This is unnecessary- we should only be returning at this point, there's no need for the additional yield.

For example:

https://github.com/google/site-kit-wp/blob/516183b8770031a75dce3d18c9e64fb1110b5be9/assets/js/googlesitekit/datastore/location/navigation.js#L63-L66

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

Acceptance criteria

Implementation Brief

assets/js/googlesitekit/datastore/location/navigation.js: 62
63: return yield { 64 type: DO_NAVIGATE_TO,

assets/js/googlesitekit/datastore/site/consent-mode.js: 152
153: return yield fetchSaveConsentModeSettingsStore.actions.fetchSaveConsentModeSettings( 154 settings

assets/js/googlesitekit/datastore/site/conversion-tracking.js: 91
92: return yield fetchSaveConversionTrackingSettingsStore.actions.fetchSaveConversionTrackingSettings( 93 settings

assets/js/googlesitekit/datastore/site/html.js: 126 *checkForSetupTag() { 127: return yield { 128 payload: {},

assets/js/googlesitekit/datastore/ui/ui.js: 55
56: return yield actions.setValue( 57 'useInViewResetCount',

99
100: return yield actions.setValue( 101 'activeOverlayNotification',

132 ) { 133: return yield actions.setValues( { 134 activeOverlayNotification: undefined,

assets/js/googlesitekit/datastore/user/adblocker.js: 40 *checkAdBlocker() { 41: return yield { 42 payload: {},

assets/js/googlesitekit/datastore/user/dismissed-items.js: 124 const { expiresInSeconds = 0 } = options; 125: return yield fetchDismissItemStore.actions.fetchDismissItem( 126 slug,

assets/js/googlesitekit/datastore/user/feature-tours.js: 124 // Dispatch a request to persist and receive updated dismissed tours. 125: return yield fetchDismissTourStore.actions.fetchDismissTour( slug ); 126 }

assets/js/googlesitekit/datastore/user/prompts.js: 100
101: return yield fetchDismissPromptStore.actions.fetchDismissPrompt( 102 slug,

assets/js/googlesitekit/datastore/user/surveys.js: 155 function* ( triggerID, timeout ) { 156: return yield fetchSetSurveyTimeoutStore.actions.fetchSetSurveyTimeout( 157 triggerID,

assets/js/googlesitekit/modules/datastore/settings.js: 50 function* ( slug ) { 51: return yield { 52 type: SUBMIT_MODULE_CHANGES,

69 function* ( slug ) { 70: return yield { 71 type: ROLLBACK_MODULE_CHANGES,

assets/js/googlesitekit/notifications/datastore/notifications.js: 154
155: return yield registry 156 .dispatch( CORE_USER )

assets/js/modules/analytics-4/datastore/enhanced-measurement.js: 249
250: return yield commonActions.await( 251 registry

311
312: return yield fetchUpdateEnhancedMeasurementSettingsStore.actions.fetchUpdateEnhancedMeasurementSettings( 313 propertyID,

assets/js/modules/analytics-4/datastore/properties.js: 353
354: return yield commonActions.await( 355 registry

455 ) { 456: return yield commonActions.await( 457 registry

506 ) { 507: return yield commonActions.await( 508 registry

Test Coverage

QA Brief

Changelog entry

eugene-manuilov commented 2 months ago

IB ✔️ but I think the estimate is a bit low because we may need additional time to broken tests. I'll bump it to 7.

techanvil commented 1 month ago

This has been moved to the Backlog, the AC will need to be revised as discussed on Slack - it looks like most of the return yield cases are indeed needed with the code as it stands, although we should update the redundant cases, but there's also a question about whether this is the correct pattern to follow, returning a non-action type from an action generator.