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

The Big Migration Thread #20

Open
zth opened this issue Feb 9, 2023 · 5 comments
Open

The Big Migration Thread #20

zth opened this issue Feb 9, 2023 · 5 comments

Comments

@zth
Copy link
Collaborator

zth commented Feb 9, 2023

Post your issues and/or gotchas from migrating in here. We'll try and help out as much as we can.

@whitchapman
Copy link

whitchapman commented Feb 9, 2023

    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
Copy link
Collaborator Author

zth commented Feb 10, 2023

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.

@Minnozz
Copy link
Contributor

Minnozz commented Feb 10, 2023

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
Copy link
Collaborator Author

zth commented Feb 10, 2023

@Minnozz fantastic and insightful write up, thank you!

@vasco3
Copy link

vasco3 commented Feb 14, 2023

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.

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

No branches or pull requests

4 participants