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

feat: support native Error causes #141

Closed
wants to merge 7 commits into from

Conversation

legobeat
Copy link
Contributor

@legobeat legobeat commented May 13, 2024

Alternative/follow-up to #140.

This utilizes the runtime Error constructor parameter options.cause to propagate data.cause. The explicit assignment of the property in #140 is preserved as fallback, which should make this change non-breaking.

This requires change tsconfig lib from ES2020 to ES2022 to be recognized. Alternatively, if allowing es2022 lib is not desired at this point, the cause prop could be explicitly added/shadowed like in #140.

Related

Based on

metamask-extension preview branch

@legobeat legobeat changed the title Propagate data cause es2022 feat: support native Error causes May 13, 2024
functions: 94.44,
lines: 92.85,
statements: 92.85,
branches: 91.89,
Copy link
Contributor Author

@legobeat legobeat May 13, 2024

Choose a reason for hiding this comment

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

The reductions here arise from the fallback never triggering since native causes are not used in all tested environments.

Adding test-coverage for the fallback would involve some wider changes to the test suite, I believe.

Copy link
Member

@Mrtenz Mrtenz left a comment

Choose a reason for hiding this comment

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

I think this makes sense. When serialised, the cause is still serialised as data.cause, right?

@legobeat
Copy link
Contributor Author

legobeat commented May 16, 2024

I think this makes sense. When serialised, the cause is still serialised as data.cause, right?

That's the idea, yes!

rekmarks
rekmarks previously approved these changes May 27, 2024
Copy link
Member

@rekmarks rekmarks left a comment

Choose a reason for hiding this comment

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

LGTM.

@legobeat legobeat requested a review from a team May 27, 2024 20:50
tsconfig.json Outdated Show resolved Hide resolved
@legobeat legobeat requested a review from a team May 27, 2024 21:48
@@ -3,7 +3,7 @@
"esModuleInterop": true,
"exactOptionalPropertyTypes": true,
"forceConsistentCasingInFileNames": true,
"lib": ["ES2020"],
"lib": ["ES2022"],
Copy link
Contributor

@mcmire mcmire May 28, 2024

Choose a reason for hiding this comment

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

Hmm, I'm not sure we can do this yet due to React Native not supporting ES2020+. There is a thread in chat about this. Search for "ES2020"; it should be the third message down. (I'll DM it to you.)

These changes look good to me otherwise, though.

Copy link
Contributor Author

@legobeat legobeat May 29, 2024

Choose a reason for hiding this comment

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

@mcmire Since es2022 is not an option, which of the following is preferred?

@legobeat legobeat requested a review from mcmire May 29, 2024 01:59
@legobeat
Copy link
Contributor Author

Closing in favor of #140

@legobeat legobeat closed this May 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants