storyblok / storyblok-js-client

Universal JavaScript client for Storyblok's API
MIT License
129 stars 87 forks source link

Breaking change with errors returned #641

Closed Cherry closed 1 year ago

Cherry commented 1 year ago

Expected Behavior

Previously, you would receive errors that were an object with a message, status, and response attached. This was great for determining if something was a 404, etc. programmatically. This is documented at https://github.com/storyblok/storyblok-js-client#error-handling.

Current Behavior

Now, errors are a JSON stringified payload that contains message, status, and response, but this drastically changes how you handle errors, requiring you to JSON.parse it now before you can determine the issue.

New: Error: {"message":"Not Found","status":404,"response":"This record could not be found"} (string) Previous: Error: Not Found (error, with structure as defined at https://github.com/storyblok/storyblok-js-client#error-handling)

Example

Pseudo examples, but illustrate the issue:

Previous code (5.14.1):

try {
    const story = await Storyblok.get(...);
    return story;
} catch(err) {
    if(err?.status === 404){
        // handle 404
    }
    throw err;
}

New code (5.14.2):

try {
    const story = await Storyblok.get(...);
    return story;
} catch(err) {
    if(typeof(err) === 'string'){
        try {
            const json = JSON.parse(err);
            if(json?.status === 404){
                // handle 404
            }
        } catch {}
    }
    throw err;
}

This massive change to how errors are returned is a breaking change and appears to have been done in https://github.com/storyblok/storyblok-js-client/pull/639. The previous method feels much more ergonomic to me than parsing a JSON string for what it's worth, but if this was intentional, could it please be rolled into a new major release as a breaking change (v6)?

thiagosaife commented 1 year ago

@Cherry We had to change this because we weren't delivering the entire object with a message, the status and the response from the server. However we'll parse the response prior to deliver it. I'll keep you posted here.

Cherry commented 1 year ago

@Cherry We had to change this because we weren't delivering the entire object with a message, the status and the response from the server. However we'll parse the response prior to deliver it. I'll keep you posted here.

Thank you for the update. I understand wanting to improve this, but as this structure is documented for error handling and in use by people using this library, please bump a major release for v6 if the type/structure is changed in a non backwards-compatible way.

thiagosaife commented 1 year ago

@Cherry You're right. I'll revert the issue on version 5 and release a major with the new error handling. Thanks for the heads up.

thiagosaife commented 1 year ago

@Cherry It was reverted. We're also launching a version 6. But dependabot won't automatically merge it. I'll close this issue, but feel free to open another one if you need. Thanks again, sir!

Cherry commented 1 year ago

Thank you very much!