rescript-compiler icon indicating copy to clipboard operation
rescript-compiler copied to clipboard

Published 20 hours ago • verified publisher icon rescript-lang

Throw as instance of JS `new Error()`

Open aspeddro opened this issue 1 year ago • 6 comments

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

Breaking Changes

Adapted from https://github.com/melange-re/melange/pull/1036

Thanks @anmonteiro

aspeddro avatar Feb 05 '24 05:02 aspeddro

Should it be somehow marked as a ReScript exception?

DZakh avatar Feb 05 '24 09:02 DZakh

Ah, I see there's a discussion exactly about this in the Melange PR https://github.com/melange-re/melange/pull/1036#issuecomment-1917749133

DZakh avatar Feb 05 '24 09:02 DZakh

Related issue: https://github.com/rescript-lang/rescript-compiler/issues/4898

fhammerschmidt avatar Feb 05 '24 09:02 fhammerschmidt

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

aspeddro avatar Feb 10 '24 00:02 aspeddro

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

instanceof support

aspeddro avatar Feb 10 '24 00:02 aspeddro

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

tsnobip avatar Feb 10 '24 07:02 tsnobip

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

cometkim avatar Feb 27 '24 16:02 cometkim

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

zth avatar Feb 27 '24 16:02 zth

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?

cristianoc avatar Feb 27 '24 17:02 cristianoc

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

zth avatar Mar 27 '24 13:03 zth

@aspeddro Could you rebase?

cknitt avatar May 30 '24 09:05 cknitt

Rebase done!! Are we going to merge this?

I need to update the changelog

aspeddro avatar May 30 '24 16:05 aspeddro

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

We can investigate whether to port https://github.com/melange-re/melange/pull/1043 separately later on.

cknitt avatar May 30 '24 18:05 cknitt

CHANGELOG updated https://github.com/rescript-lang/rescript-compiler/pull/6611/commits/15ea2538ffd8491aef2eec8e2527af5e9a6219b2

aspeddro avatar May 30 '24 18:05 aspeddro

Good to go from my end.

zth avatar May 30 '24 18:05 zth

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.

cknitt avatar May 30 '24 19:05 cknitt

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.

  • https://www.npmjs.com/package/invariant
  • https://www.npmjs.com/package/ts-invariant

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

cometkim avatar Jul 25 '24 12:07 cometkim