Skip to content
This repository has been archived by the owner on Jun 15, 2023. It is now read-only.

Add loc for returned component body #633

Merged
merged 3 commits into from
Sep 14, 2022
Merged

Add loc for returned component body #633

merged 3 commits into from
Sep 14, 2022

Conversation

mununki
Copy link
Member

@mununki mununki commented Sep 14, 2022

This PR fixes the issue rescript-lang/rescript#5635
It show the error with loc now.

image

@cristianoc
Copy link
Contributor

cristianoc commented Sep 14, 2022

This screenshot is with V3.

@mununki mununki changed the title add loc for returned component body Add loc for returned component body Sep 14, 2022
@cristianoc
Copy link
Contributor

@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:


  We've found a bug for you!
  /Users/cristianocalcagno/GitHub/rescript-vscode/analysis/tests/src/Tst.res:10:5-14

   8 │     let \"Tst$Foo" = (props: props<_>) => make(props)
   9 │ 
  10 │     \"Tst$Foo".    <-------
  11 │   })
  12 │ }

  This has type: props<string> => React.element
  Somewhere wanted:
    React.component<({.."foo": 'b} as 'a)> (defined as 'a => React.element)
  
  The incompatible parts:
    props<string> vs 'a

@cristianoc
Copy link
Contributor

Alternative JSX behaviour (don't let-define Tst$Foo):

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)
  })
}

Error:
Screenshot 2022-09-14 at 09 34 30


@cristianoc
Copy link
Contributor

The second one is clearer, but I'm not sure how clearly it can be expressed w.r.t. the original code.

@cristianoc
Copy link
Contributor

@mattdamon108 I'm thinking of merging the PR as it is now.
Thoughts?

@mununki
Copy link
Member Author

mununki commented Sep 14, 2022

Alternative JSX behaviour (don't let-define Tst$Foo):

Can you show me the js output? The lef-define such as Tst$Foo is a secret recipe to make the js output function name starting with the capital letter.

Copy link
Contributor

@cristianoc cristianoc left a 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.

@mununki
Copy link
Member Author

mununki commented Sep 14, 2022

Sorry for missing to add tests 😓 I won't forget next time.

@cristianoc
Copy link
Contributor

cristianoc commented Sep 14, 2022

Alternative JSX behaviour (don't let-define Tst$Foo):

Can you show me the js output? The lef-define such as Tst$Foo is a secret recipe to make the js output function name starting with the capital letter.

That magic does nothing useful when the top-level of the body is an application: it simply gets lost.

@cristianoc cristianoc merged commit 60a9dc7 into master Sep 14, 2022
@cristianoc cristianoc deleted the imprv-error-loc branch September 14, 2022 07:48
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants