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

Throw as instance of JS new Error() #6611

Merged
merged 7 commits into from
May 30, 2024

Conversation

aspeddro
Copy link
Contributor

@aspeddro aspeddro commented Feb 5, 2024

Throws an instance of JavaScript's new Error() and adds the extension payload for cause option

Breaking Changes

Adapted from melange-re/melange#1036

Thanks @anmonteiro

@DZakh
Copy link
Contributor

DZakh commented Feb 5, 2024

Should it be somehow marked as a ReScript exception?

@DZakh
Copy link
Contributor

DZakh commented Feb 5, 2024

Ah, I see there's a discussion exactly about this in the Melange PR melange-re/melange#1036 (comment)

@fhammerschmidt
Copy link
Member

Related issue: #4898

@aspeddro
Copy link
Contributor Author

Ready for review. I don't know if it should go to v12 or v11

@aspeddro
Copy link
Contributor Author

Since cause is only supported above Node 16.9 we can emit RescriptError and use instanceof

instanceof support

@tsnobip
Copy link
Contributor

tsnobip commented Feb 10, 2024

Can't we just drop support for nodejs 16 and only support node > 18? They dropped support for node 16 last August already.

@cometkim
Copy link
Member

Can't we just drop support for nodejs 16 and only support node > 18? They dropped support for node 16 last August already.

related: #6429

@zth
Copy link
Collaborator

zth commented Feb 27, 2024

Sorry, missed this. Looks like a good change! @cristianoc any thoughts?

@cristianoc
Copy link
Collaborator

Would be useful to have an assessment of the consequences of this change, as well as a description.
I guess this changes the internal representation of exceptions raised from ReScript.
First things that come to mind is if this affects the ability of recognising what exceptions are from ReScript, and e.g. pattern match on them.
Are there any observable changes after this in user code, any breaks, etc?

@zth
Copy link
Collaborator

zth commented Mar 27, 2024

@aspeddro and anyone else interested - some more research needed here, let us know if you're interested in tackling it.

@cknitt
Copy link
Member

cknitt commented May 30, 2024

@aspeddro Could you rebase?

@aspeddro
Copy link
Contributor Author

Rebase done!! Are we going to merge this?

I need to update the changelog

@cknitt
Copy link
Member

cknitt commented May 30, 2024

LGTM and could be merged after after updating the CHANGELOG.
Agreed @zth?

We can investigate whether to port melange-re/melange#1043 separately later on.

@aspeddro
Copy link
Contributor Author

CHANGELOG updated 15ea253

@zth
Copy link
Collaborator

zth commented May 30, 2024

Good to go from my end.

@cknitt
Copy link
Member

cknitt commented May 30, 2024

Was wondering why CI wouldn't run, but then noticed the [skip ci] on the CHANGELOG commit. 😄
Will merge now as CI was successful on the previous commit.

@cometkim
Copy link
Member

With large scale codebases (especially web clients) this is a bit of an issue. Error usage is a major cause of bundle bloat, and major libraries such as React and Apollo additionally use a system called "invariant" to reduce bundle size bloating by errors.

As we own the toolchain, there remains the potential to create a much better & performant result.

aspeddro added a commit to aspeddro/rescript-compiler that referenced this pull request Sep 6, 2024
@aspeddro aspeddro mentioned this pull request Sep 6, 2024
cknitt pushed a commit that referenced this pull request Sep 6, 2024
* Revert #6611

* format

* update CHANGELOG.md
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.

8 participants