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

Add optional pre-request hook to Axios fetcher #341

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

rafalkosla101
Copy link
Contributor

Fixed the bug where error 404 was not parsed as spree error, extended sdk to have custom function that can be specified by user

@rafalkosla101 rafalkosla101 mentioned this pull request Aug 5, 2022
@kshalot
Copy link
Member

kshalot commented Aug 8, 2022

A couple of quick comments:

  1. Since the parsing issue was moved to a separate PR (Fix error parsing #343) we could remove it here as well. Even though it won't matter if the other PR gets merged first, there's no point forcing a particular order here. You could also squash the commits into something more precise, e.g. Add optional pre-request hook parameter to Axios fetcher.
  2. This functionality could probably be tested. The tests of this SDK are pretty concise but maybe there's a nice place where this could live.
  3. Now that I think of it, the locale functionality is going to become part of Spree core. So it would probably be better if we come up with a mechanism that will allow you to explicitly set the locale instead of this very open pre-request hook function. Due to Vuestorefronts architecture this will probably boil down to a very similar approach (because of the server proxy) but maybe there are some better approaches. E.g. since VSF supports the vsf-locale cookie then it makes sense for it to support passing it to the backend somehow as well (might be tricky though because of all the abstractions, I'll think about it)

@kshalot kshalot changed the title Add extension to sdk Add optional pre-request hook to the Axios fetcher Aug 8, 2022
@kshalot kshalot changed the title Add optional pre-request hook to the Axios fetcher Add optional pre-request hook to Axios fetcher Aug 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants