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 future.v2_errorBoundary flag #4918

Merged
merged 13 commits into from
Jan 13, 2023
Merged

Conversation

brophdawg11
Copy link
Contributor

Adds future.v2_errorBoundary flag to opt into Remix v2 ErrorBoundary behavior

@changeset-bot
Copy link

changeset-bot bot commented Dec 19, 2022

🦋 Changeset detected

Latest commit: 9aa0dc1

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 18 packages
Name Type
@remix-run/react Patch
@remix-run/server-runtime Patch
@remix-run/testing Patch
@remix-run/cloudflare Patch
@remix-run/deno Patch
@remix-run/dev Patch
@remix-run/node Patch
@remix-run/cloudflare-pages Patch
@remix-run/cloudflare-workers Patch
create-remix Patch
@remix-run/css-bundle Patch
@remix-run/architect Patch
@remix-run/express Patch
@remix-run/netlify Patch
@remix-run/vercel Patch
@remix-run/serve Patch
remix Patch
@remix-run/eslint-config Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

return <RemixRootDefaultCatchBoundaryImpl caught={caught} />;
}

function RemixRootDefaultCatchBoundaryImpl({
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Extracted internals into a component that can be used by the default Remix v2 Root Error Boundary

differentiateCatchVersusErrorBoundaries(build, context);
if (build.future.v2_errorBoundary !== true) {
differentiateCatchVersusErrorBoundaries(build, context);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

No need to re-bubble if we only have one element

// @ts-expect-error
return <ErrorBoundary />;
}
throw error;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we've opted in, just render the ErrorBoundary directly

Comment on lines +180 to +193
if (init.future) {
let contents = fse.readFileSync(
path.join(projectDir, "remix.config.js"),
"utf-8"
);
if (!contents.includes("future: {},")) {
throw new Error("Invalid formatted remix.config.js in template");
}
contents = contents.replace(
"future: {},",
"future: " + JSON.stringify(init.future) + ","
);
fse.writeFileSync(path.join(projectDir, "remix.config.js"), contents);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you pass a future config into createFixtureProject we'll write it into the remix config here

error={tError.error}
/>
);
return <ErrorBoundary error={tError.error} />;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got rid of the Remix Error Boundary wrapper since we can just use on the RR one internally

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jacob-ebey This is what I was talking about. From my testing all seems fine here

Comment on lines +58 to +63
let errorString =
error == null
? "Unknown Error"
: typeof error === "object" && "toString" in error
? error.toString()
: JSON.stringify(error);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this worth it, maybe we just console.error and fall back to new Error("Unknown Error")?

@brophdawg11
Copy link
Contributor Author

I gotta figure out what's going on with this TSC error in the build :/. Can't seem to pull AppConfig in from remix-dev

@mcansh
Copy link
Collaborator

mcansh commented Dec 23, 2022

I gotta figure out what's going on with this TSC error in the build :/. Can't seem to pull AppConfig in from remix-dev

depending on if you're generating type defs, you'll need to delete the dist directory from remix-dev trash ./packages/remix-dev/dist

packages/remix-react/routeModules.ts Outdated Show resolved Hide resolved
brophdawg11 and others added 2 commits January 11, 2023 17:38
Co-authored-by: Michaël De Boey <info@michaeldeboey.be>
@jacob-ebey jacob-ebey merged commit ef66307 into dev Jan 13, 2023
@jacob-ebey jacob-ebey deleted the brophdawg11/v2-error-boundary branch January 13, 2023 23:14
@github-actions github-actions bot added the awaiting release This issue has been fixed and will be released soon label Jan 13, 2023
@github-actions
Copy link
Contributor

🤖 Hello there,

We just published version 1.11.0-pre.0 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

@github-actions
Copy link
Contributor

🤖 Hello there,

We just published version v0.0.0-nightly-d593e4a-20230114 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

@github-actions github-actions bot removed the awaiting release This issue has been fixed and will be released soon label Jan 18, 2023
@github-actions
Copy link
Contributor

🤖 Hello there,

We just published version 1.11.0 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants