rescript-association/rescript-core

The Big Migration Thread

zth opened this issue · 5 comments

zth commented

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.

zth commented

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.

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 of RescriptCore 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 -opens, 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 to String.split)
  • After noticing this I decided to remove the Js. prefix everywhere
  • Belt.Array.concatMany (which was Array.concatMany in our codebase) and RescriptCore.Array.concatMany (which is now Array.concatMany) have the same name but different signatures; I had to migrate them to RescriptCore's Array.flat
  • Belt.Array.joinWith had an extra 't => string parameter which RescriptCore.Array.joinWith does not have (an improvement in my opinion; all but one place in our codebase passed x => x)
  • Belt.Array.mapWithIndex passes the index as the first argument, while RescriptCore.Array.mapWithIndex passes the index as the second argument
  • Belt.Array.slice takes ~offset and ~len, while RescriptCore.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, versus RescriptCore.String.substring (~start and ~end)
  • Belt.Array.sliceToEnd takes a positional argument while RescriptCore.Array.sliceToEnd takes a labeled argument (~start)
  • I decided to use Array.getUnsafe instead of Array.getExn (which does not exist anymore in RescriptCore) in some places
  • There now exists a module Date and module Error in global scope, which are shadowed by some of my own modules
  • I passed Belt.Int.fromString to Array.map, but RescriptCore.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 if findIndex returned an option<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 functions
    • Belt.Map.Int and functions
    • Belt.Set and functions (extensively)
    • Belt.Set.String and functions
    • Belt.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
zth commented

@Minnozz fantastic and insightful write up, thank you!

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.