Add throw
#7346
Add throw
#7346
Conversation
runtime/Stdlib_Exn.res
Outdated
There was a problem hiding this comment.
Should we add @@deprecated to this module?
It has the same content as https://github.com/rescript-lang/rescript/blob/master/runtime/Stdlib_Error.res
There was a problem hiding this comment.
it doesn't have exactly the same content though, Error is missing the definition of a JS exception that today is catched with Exn.Error(obj), but I agree the current situation is not ideal, maybe we could merge the two?
There was a problem hiding this comment.
I will explore this in another PR
There was a problem hiding this comment.
thanks for doing this @aspeddro, I think that's a nice improvement. I'm just not sure if we should also deprecate raise, having both without any explanation feels a bit weird to me. I'm also not sure about what we should do for Stdlib.Exn module, but I'd advise to keep this work for another PR to avoid bikeshedding on this topic.
runtime/Pervasives_mini.res
Outdated
| @@ -1,5 +1,6 @@ | |||
| /* Exceptions */ | |||
| external raise: exn => 'a = "%raise" | |||
There was a problem hiding this comment.
should we deprecate raise or keep both for now?
|
We can add to deprecated attribute?
|
runtime/Stdlib.res
Outdated
| @@ -100,6 +100,7 @@ async function main() { | |||
| external import: 'a => promise<'a> = "%import" | |||
|
|
|||
| let panic = Error.panic | |||
| let throw = Error.throw | |||
There was a problem hiding this comment.
This is not zero-cost (causes a $$throw function to be emitted in Stdlib.js). I am afraid we need to repeat the external definition here.
runtime/Stdlib.res
Outdated
| } | ||
| ``` | ||
| */ | ||
| external throw: Error.t => 'a = "%raise" |
There was a problem hiding this comment.
Should I add the external here or just leave it in the Stdlib_Error.res module?
There was a problem hiding this comment.
Maybe clearer to add it in Pervasives on top of the deprecated raise instead?
|
CHANGELOG updated |
|
@aspeddro did you see my comment?
|
Yes, I will adjust |
|
Should we also have a |
Good idea. |
Deprecate
raiseand rename references.Close #7113