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

useAsyncStoryblok status code checking on response, throws an error on error.value?.response.status #329

Closed
Dru-S opened this issue Mar 13, 2023 · 20 comments · Fixed by #398
Assignees
Labels
bug [Issue] Something isn't working

Comments

@Dru-S
Copy link

Dru-S commented Mar 13, 2023

Expected Behavior

The status property must be checked not only inside the error.value.response object, but also directly inside the error.value property of the response:

...

const status = error.value?.response?.status || error.value?.status || false;
  if (status && status >= 400 && status < 600) {
    const message = error.value?.message.message || error.value?.message || '';
    throw createError({ statusCode: status, statusMessage: message });

...

Current Behavior

The status property is only checked inside the error.value.response, throwing a caught (in promise) TypeError: Cannot read properties of undefined (reading 'status') error.

...

if (error.value?.response.status >= 400 && error.value?.response.status < 600) {
  throw createError({ statusCode: error.value?.response.status, statusMessage: error.value?.message.message });

...

Steps to Reproduce

  1. Simply try to get a 404 request with the useAsyncStoryblok

Additional informations

├── @storyblok/[email protected]
├─┬ @storyblok/[email protected]
│ └─┬ @storyblok/[email protected]
│   └── @storyblok/[email protected] deduped
└── [email protected]
@SebbeJohansson
Copy link
Contributor

Hi @Dru-S! This PR will fix this issue. #316

@Dawntraoz
Copy link
Contributor

We've already merged the fix from @SebbeJohansson 🎉
@Dru-S can you please download the latest SDK version and let us know if everything works as expected?

In case you face any other issue, let us know creating a new one <3

@Dawntraoz Dawntraoz self-assigned this Apr 19, 2023
@Dawntraoz Dawntraoz added the bug [Issue] Something isn't working label Apr 19, 2023
@Dru-S
Copy link
Author

Dru-S commented Apr 19, 2023

Hi @Dawntraoz, thanks for the update.

I updated to the latest version of the @storyblok/nuxt package only, but unfortunately it still didn't work as expected.
I also update Nuxt to the latest version, and this introduces a new bug since it seems that in the useAsyncData they added a createError, and the check for the status code inside the useAsyncStoryblok it's broken, again.

Here's the screenshot of the diff, for the new createError of Nuxt's useAsyncData:
SCRNSHT 2023-04-19 alle 16 57 22

So, as per my suggestion on the issue, integrating the PR #316 of @SebbeJohansson, this snippets will solve the problem, with the addition of having the compatibility with older and latest versions of Nuxt:

const status = error.value?.response?.status || error.value?.status || error.value?.statusCode || false;
if (status >= 400 && status < 600) {
	const message = error.value?.message.message || error.value?.message || error.value?.statusMessage || 'Something went wrong when fetching from storyblok.';
	throw createError({ statusCode: status, statusMessage: message });
}

Thanks!

@SebbeJohansson
Copy link
Contributor

huh interesting! What exact versions did you test this on and do you have some code I can test it with?
Im trying to replicate it on my own in the meantime.

@SebbeJohansson
Copy link
Contributor

SebbeJohansson commented Apr 20, 2023

I can replicate it!
Regarding your suggestion, there are a few things that we might be confused about!

  1. error.value contains the error object from storyblok, and not a nuxtError. So it does not contain a value called statusCode.
    Example:
    image
    image
  2. Same with statusMessage.
    image
  3. Since storyblokApiInstance.get returns an ISbError-object, then we don't have a string inside of error.value?.message but rather, it is an Error type.
  4. The current composable doesn't seem to return the correct type at all now.

Number 3 might be something that should be changed in the js-client, but the others we can probably handle in this!

My suggestion would be something like:

  const { data, error } = await useAsyncData<ISbResult, ISbError>(
    `${uniqueKey}-asyncdata`,
    () => storyblokApiInstance.get(`cdn/stories/${url}`, apiOptions),
  );
  const status = error.value?.response?.status || error.value?.status || (error.value as any)?.statusCode || 500;
  if (status >= 400 && status < 600) {
    const message = error.value?.message?.message || error.value?.message?.name || `${error.value?.message}` || 'Something went wrong when fetching from storyblok.';
    throw createError({ statusCode: status, statusMessage: message });
  }

The error object for a proper 404 from storyblok would be:
image

This still doesn't fix the issue where we are not returning the correct error type. I need to think more about this.

What do you think? I added a fallback to 500 since that's just a general server error that can correspond to "Something went wrong when fetching from storyblok." since we don't know what happened.

@Dawntraoz Can you open this again?

@Dawntraoz Dawntraoz reopened this Apr 20, 2023
@Dawntraoz
Copy link
Contributor

@SebbeJohansson reopened, are you planning to work on it, or shall I? Let me know if you need any help.

PS: It will be also nice to document the error handling in the README.md, if you can add it to your PR that would be amazing!

@SebbeJohansson
Copy link
Contributor

@Dawntraoz Im trying to look at it, but I think this might be too much for me 😅

@Dru-S
Copy link
Author

Dru-S commented Apr 20, 2023

@SebbeJohansson yes I think it would be a correct approach, but onestly the only thing I'm not sure about it's the ISbError type of the error returned by useAsyncData.
After debugging a bit, in my case, the error is triggered by the createError inside the .finally() of the useAsyncData:
https://github.com/nuxt/nuxt/blob/8234bc18d7048a7871605f207b1e7d98396c1128/packages/nuxt/src/app/composables/asyncData.ts#L172-L181

So inside the error const, there is a statusCode:
image

I cannot reproduce the example in which there's no statusCode inside, maybe we have different versions of the dependencies; here's mine:

❯ npm ls nuxt
└── [email protected]

❯ npm ls @storyblok/nuxt
└── @storyblok/[email protected]

I don't know if I understand correctly what you have written, but why do you need to cast the useAsyncData return const to ISbResult, ISbError?

@SebbeJohansson
Copy link
Contributor

@Dru-S we have the same versions so that's good! I can reproduce the same issue using just purely the js-client without the nuxt3 module so I think useAsyncData is casting the error automatically to a nuxt3 error.
Example here (where the path I'm fetching from obviously doesn't exist so it returns 404):
image
image

The casting seems to be completely useless since useAsyncData seems to have recently been changed so that it is messing with the error return (or I was completely lost when I made the changes to cast to the ISbError-interface.
You can see here that it is supposed to work: https://nuxt.com/docs/api/composables/use-async-data#:~:text=type%20AsyncData%3CDataT,ErrorT%20%7C%20null%3E

Maybe it will have to be fine with just returning statusCode and the message?

@Dawntraoz
Copy link
Contributor

Then, I will ask the Nuxt 3 team for their input on our composable's error handling. I will keep you posted with the PR.

@SebbeJohansson
Copy link
Contributor

@Dawntraoz Sounds like a good idea! I did some more testing and I couldn't get it to work! 😭

@Dawntraoz
Copy link
Contributor

I got an answer from Daniel, he suggested me to use $fetch directly instead of asyncData. Also, that we wouldn't directly throw an error but return an error ref which gets filled up if there is an error.

So I will work on a PR with the changes and send it to you to test it locally before merging 🫂

Let's improve the composable to be as good as we want/need 💪

@SebbeJohansson
Copy link
Contributor

Hi! Sorry for the late response, but been on a trip and then gotten ill sadly. Hopefully I'll be back in the next day or two.

Using pure $fetch is interesting. Maybe there is a case to be made for something "easier" to be available from Nuxt, but that's a discussion for a different place.
Not sure I'm following on "don't throw an error", but I guess it would make sense to pass the info along. I know this was something we were unsure off, and that we discussed. Maybe we should have just talked to Daniel right away 😄

Sounds good. I'll try to test as soon as something is available. 🥳

@Maxino22
Copy link

Been Following up on this I may have a similar instance
my current setup is as follow in my pages I have [...slug].vue

const { slug } = useRoute().params

const resolveRelations = ['all-services.services']

const story = await useAsyncStoryblok(
	Array.isArray(slug) && slug.length > 0 ? slug.join('/') : 'home',
	{ version: 'published', resolve_relations: resolveRelations },
	{
		resolveRelations,
	}
)

I want to catch 404 urls how will i do that with the useAsynStoryblok

@SebbeJohansson
Copy link
Contributor

@Maxino22 Until Storyblok has decided on how to officially do it, I think the best idea is to try-catch it.

const {
    slug
} = useRoute().params
const resolveRelations = ['all-services.services']

const story = null;
try {
    story = await useAsyncStoryblok(
        Array.isArray(slug) && slug.length > 0 ? slug.join('/') : 'home', {
            version: 'published',
            resolve_relations: resolveRelations
        }, {
            resolveRelations,
        })
} catch (e) {
  // do something when its a 404.
}

@sumerokr
Copy link

sumerokr commented Jun 5, 2023

Hey, @SebbeJohansson but useAsyncStoryblok doesn't throw an error that can be catched (at least for 404 pages).
As of now (v 5.4.9) code looks like this

const { data, error } = await useAsyncData(
  `${uniqueKey}-asyncdata`,
  () => storyblokApiInstance.get(`cdn/stories/${url}`, apiOptions)
);

and the error that we get here is an instance of H3Error. It doesn't have response property that is checked below

if (error.value?.response && error.value?.response.status >= 400 && error.value?.response.status < 600) {
  throw createError({ statusCode: error.value?.response.status, statusMessage: error.value?.message?.message || "Something went wrong when fetching from storyblok." });
}

Therefore nothing will be thrown. useAsyncStoryblok quietly returns undefined as a story.

@sumerokr
Copy link

sumerokr commented Jun 5, 2023

Moreover, it looks that response property doesn't exist even on what storyblokApiInstance.get returns for 404 pages.
When Storyblok API response returns 404 code, its body is ["This record could not be found"]. The string from that array is used as response value. See here https://github.com/storyblok/storyblok-js-client/blob/main/src/sbFetch.ts#L165.

{
  message: Error: Not Found
  status: 404,
  response: 'This record could not be found'
}

Of course, a string doesn't have a status property and condition check

error.value?.response.status >= 400 && error.value?.response.status < 600

wouldn't catch it.

One more thing, just FYI. Let's take a look once again at the error returned from the storyblokApiInstance.get

{
  message: Error: Not Found
  status: 404,
  response: 'This record could not be found'
}

property message is not really a message. It's an instance of Error that we can visualize (for simplicity) as

{
  message: {
    name: 'Error',
    message: 'NotFound'
  },
  status: 404,
  response: 'This record could not be found'
}

you can find it here https://github.com/storyblok/storyblok-js-client/blob/main/src/sbFetch.ts#L162
I assume that line should be message: new Error(res.statusText).message, or just message: res.statusText,.
Why would one call an Error instance as message?

@Maxino22
Copy link

Maxino22 commented Jun 6, 2023

Been Following up on this I may have a similar instance my current setup is as follow in my pages I have [...slug].vue

const { slug } = useRoute().params

const resolveRelations = ['all-services.services']

const story = await useAsyncStoryblok(
	Array.isArray(slug) && slug.length > 0 ? slug.join('/') : 'home',
	{ version: 'published', resolve_relations: resolveRelations },
	{
		resolveRelations,
	}
)

I want to catch 404 urls how will i do that with the useAsynStoryblok

Hry @sumerokr for a case of using useAsyncStoryblok the solution to create a 404 error would it be to try catch the undefined returned or i do not quite understand??

@sumerokr
Copy link

sumerokr commented Jun 6, 2023

@Maxino22 based on my experience that I have described above, 404 error will not be thrown. Therefore it cannot be catched.
As a workaround we could

  • check if resolved story is undefined // feels too dirty and you can't be sure, that it's really a 404 error. Could be whatever else.
  • use storyblokApiInstance.get directly, it will throw an error that can be catched
  • write your own composable similar to node_modules/@storyblok/nuxt/dist/runtime/composables/useAsyncStoryblok.mjs with fixed error handling
  • fix, submit a PR and wait SB team to merge it

@SebbeJohansson
Copy link
Contributor

@Dawntraoz any updates on this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug [Issue] Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants