The Big Migration Thread
zth opened this issue · 5 comments
Post your issues and/or gotchas from migrating in here. We'll try and help out as much as we can.
let msg = switch e {
| JsError(err) =>
switch Exn.message(err) {
| Some(msg) => msg
| None => ""
}
| _ => "Unexpected error occurred"
}
After the migration to @rescript/core, the above code fragment has e
with type exn
and err
now with type unknown
which results in the following compile error in reference to err
being passed into Exn.message
:
This has type: unknown
Somewhere wanted: RescriptCore.Exn.t (defined as Js_exn.t)
This worked before the migration. Thanks.
EDIT: I modified JsError
to Exn.Error
and it compiled, but I don't think this code properly handles Js exceptions. I looked in the rescript documentation and did not find much.
EDIT: I modified
JsError
toExn.Error
and it compiled, but I don't think this code properly handles Js exceptions. I looked in the rescript documentation and did not find much.
This is the correct way to do it. Does it not work for you? We have tests with that same behavior: https://github.com/rescript-association/rescript-core/blob/main/test/PromiseTest.res#L46-L71
This is a good gotcha for anyone already using rescript-promise
and I'll add it to the migration readme as soon as we've sorted this out.
EDIT: More context. rescript-promise
used to define its own JsError
exception because at the time the compiler didn't have the necessary internal APIs to handle JS exceptions the way rescript-promise
wanted. These internal APIs have shipped since then, so Core
uses them instead, which is the way going forward.
I migrated our entire corporate code base in a branch (for now) to see what came up.
Our starting point was -open Belt
and -open ReScriptJs
(bloodyowl's).
I decided to switch to -open RescriptCore
only and not open Belt, to make it clearer where we still use Belt.
Some observations (not issues in my opinion):
- I had expected
ReScriptCore
instead ofRescriptCore
to be the name of the module, consistent with project branding - Because we already used bloodyowl's rescript-js and
-open Belt
, I assumed we could not use the automated migrations. If this is incorrect, we may want to add a note about this for other users. - Because of our
-open
s,Js.String.Split
used to refer to bloodyowl's binding and now refers to the binding shipped with the compiler, which has the same signature but a different implementation. This caused erroneous behaviour at runtime (fixed by changing toString.split
) - After noticing this I decided to remove the
Js.
prefix everywhere Belt.Array.concatMany
(which wasArray.concatMany
in our codebase) andRescriptCore.Array.concatMany
(which is nowArray.concatMany
) have the same name but different signatures; I had to migrate them to RescriptCore'sArray.flat
Belt.Array.joinWith
had an extra't => string
parameter whichRescriptCore.Array.joinWith
does not have (an improvement in my opinion; all but one place in our codebase passedx => x
)Belt.Array.mapWithIndex
passes the index as the first argument, whileRescriptCore.Array.mapWithIndex
passes the index as the second argumentBelt.Array.slice
takes~offset
and~len
, whileRescriptCore.Array.slice
takes~start
and~end
, which can be a gotcha- Same for
ReScriptJs.String.substr
(~start
and~length
), which is not present in RescriptCore, versusRescriptCore.String.substring
(~start
and~end
) Belt.Array.sliceToEnd
takes a positional argument whileRescriptCore.Array.sliceToEnd
takes a labeled argument (~start
)- I decided to use
Array.getUnsafe
instead ofArray.getExn
(which does not exist anymore in RescriptCore) in some places - There now exists a
module Date
andmodule Error
in global scope, which are shadowed by some of my own modules - I passed
Belt.Int.fromString
toArray.map
, butRescriptCore.Int.fromString
takes a optional~radix
argument which created a warning - Some renamed types:
Js.Date.Time.t
->Date.time
- Some renamed functions:
Js.String.replaceString
->String.replace
keep
->filter
keepMap
->filterMap
getBy
->find
getIndexBy
->findIndexOpt
(would have preferred iffindIndex
returned anoption<int>
)Js.JSON.Decode.classify
->JSON.Classify.classify
Js.Global.setTimeout
->setTimeout
(don't really like these functions in the global scope)
- Places where we still use Belt after the migration:
Belt.Map
and functions (extensively)Belt.Map.String
and functionsBelt.Map.Int
and functionsBelt.Set
and functions (extensively)Belt.Set.String
and functionsBelt.Id
(for Map and Set)Belt.SortArray.stableSortBy
Belt.Array.zip
Belt.Array.unzip
Belt.Array.range
Belt.Array.partition
Belt.Array.make
Belt.Array.makeBy
I found out
Js.Math._PI
is now Math.Constants.pi
I also was using open Belt
in many files so I removed that and added Belt.
to all the Array
so combo can work better.
Array.map(
-> Belt.Array.map
Combi missed to flip the index in Array.mapWithIndex
.