-
Notifications
You must be signed in to change notification settings - Fork 10.8k
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
Migrate @woocommerce/data
options store to TS
#33168
Conversation
edd4435
to
97ebeec
Compare
const { dismissTask, undoDismissTask } = useDispatch( OPTIONS_STORE_NAME ); | ||
const { dismissTask, undoDismissTask } = useDispatch( | ||
ONBOARDING_STORE_NAME | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Found a bug with the TS check. dismissTask
and *Task
are in the onboarding store, not options store.
woocommerce/packages/js/data/src/onboarding/actions.js
Lines 356 to 375 in c83dedd
export function* dismissTask( id ) { | |
yield dismissTaskRequest( id ); | |
try { | |
const task = yield apiFetch( { | |
path: `${ WC_ADMIN_NAMESPACE }/onboarding/tasks/${ id }/dismiss`, | |
method: 'POST', | |
} ); | |
yield dismissTaskSuccess( | |
DeprecatedTasks.possiblyPruneTaskData( task, [ | |
'isDismissed', | |
'isSnoozed', | |
] ) | |
); | |
} catch ( error ) { | |
yield dismissTaskError( id, error ); | |
throw new Error(); | |
} | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great catch! I wonder how this didn't result in an error before 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps cc @joelclimbsthings @woocommerce/mothra in case if there's anything worth looking
📊 Test reports for this pull request have been published and are accessible through the following links:
Latest commit referenced in the reports: Visit the WooCommerce Test Reports homepage to view all published reports. See the FAQs page if you're having problems accessing them. |
97ebeec
to
991de47
Compare
991de47
to
4d9e07b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests well, LGTM 🚢
Hi @chihsuan, thanks for merging this pull request. Please take a look at these follow-up tasks you may need to perform:
|
All Submissions:
Changes proposed in this Pull Request:
Part of #33105
This PR migrates options stores to TS and fixes type errors in ./client after migrating options store to TS
How to test the changes in this Pull Request:
pnpm nx build @woocommerce/data
without errorspnpm nx test @woocommerce/data
without errorsOther information:
pnpm nx changelog <project>
?FOR PR REVIEWER ONLY: