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 specify exceptions for exceptions handling the vite manifest file #52169

Merged
merged 3 commits into from
Jul 18, 2024

Conversation

SamuelWei
Copy link
Contributor

@SamuelWei SamuelWei commented Jul 17, 2024

About

This PR changes the handling of errors when trying to access the resources from the manifest file so that more specific exceptions are thrown than just the default Exception.

The ViteManifestNotFoundException is already such a more specific exception and this PR wants to extend this behavior to the other two types of execution that can occur.

Backwards compatibility

The existing ViteManifestNotFoundException now extends the new ViteException so that catching the ViteManifestNotFoundException still works, as does the new ViteException. A deprecation PHDoc type has been added so that ViteManifestNotFoundException can be removed in the next major release.

Example use case

You want to create a specific error page to indicate to users that they have missed the step to build their frontend, have an incorrect configuration in Docker containers, etc. Without this function, only a general exception is thrown in production (500), but with this PR a specific error page can be created.

Example code to how it could be used

// app/Exceptions/Handler.php

public function register(): void
    {
        $this->renderable(function (ViewException $e, Request $request) {
            $previous = $e->getPrevious();
            if ($previous instanceof ViteException) {
                return response()->view('errors.frontend-missing', [], 560);
            }

            return null;
        });
}

@SamuelWei SamuelWei changed the title Add specify exceptions for exceptions handling the manifest file Add specify exceptions for exceptions handling the vite manifest file Jul 17, 2024

use Exception;

class ViteFileFromManifestNotFoundException extends Exception
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be better to extend from some common, Vite-related base exception (eg. \Illuminate\Foundation\ViteException).

This way, all Vite-related exceptions can be caught/matched by a single class.

try {
    // ...
} catch (ViteException $ex) {
    // All Vite-related exceptions
}

Copy link
Contributor Author

@SamuelWei SamuelWei Jul 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would be fine with that as well.

Should ViteException just be the new parent, or should there only exists a single ViteException for all three cases?

Should I change this PR or open an other one instead?

I could make ViteManifestNotFoundException extend this new ViteException, so that ViteManifestNotFoundException would still work for backward compatibility and at the same time it could be deprecated. What do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer to see just the single Illuminate\Foundation\ViteException class for all of these.

We don't have a usecase for catching them on an individual basis.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed it to the single Illuminate\Foundation\ViteException but keeping ViteManifestNotFoundException for backwards compatibility reasons. Searching in Github i found a few projects that depend on this.

Should we keep the return type of the manifest method or also change it to the new ViteException?

@taylorotwell taylorotwell merged commit 6c16f47 into laravel:11.x Jul 18, 2024
28 checks passed
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

4 participants