-
Notifications
You must be signed in to change notification settings - Fork 38
Conversation
|
@mattdamon108 thanks for spotting the location of the issue. Now for the error message, looking at the error after PPX. Desugared: module Foo = {
let compare = (nextProps, currentProps) => nextProps["foo"] == currentProps["foo"]
let memo = React.memoCustomCompareProps(_, compare)
type props<'foo> = {key?: string, foo: 'foo}
let make = ({foo, _}: props<string>) => React.string(foo)
let make = memo({
let \"Tst$Foo" = (props: props<_>) => make(props)
\"Tst$Foo"
})
} Error:
|
Alternative JSX behaviour (don't let-define module Foo = {
let compare = (nextProps, currentProps) => nextProps["foo"] == currentProps["foo"]
let memo = React.memoCustomCompareProps(_, compare)
type props<'foo> = {key?: string, foo: 'foo}
let make = ({foo, _}: props<string>) => React.string(foo)
let make = memo({
(props: props<_>) => make(props)
})
}
|
The second one is clearer, but I'm not sure how clearly it can be expressed w.r.t. the original code. |
@mattdamon108 I'm thinking of merging the PR as it is now. |
Can you show me the js output? The lef-define such as |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the fix!
Some discussion in the conversation part of the PR.
If anything, we can tackle that later on.
Sorry for missing to add tests 😓 I won't forget next time. |
That magic does nothing useful when the top-level of the body is an application: it simply gets lost. |
This PR fixes the issue rescript-lang/rescript#5635
It show the error with loc now.