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

Fix jsx V4 issue with type constraints and components names other than "make". #617

Merged
merged 8 commits into from
Jul 22, 2022

Conversation

mununki
Copy link
Member

@mununki mununki commented Jul 21, 2022

Fix issues in #615

  • Conversion for the alias
  • Backward compatibility for passing the optional expression to prop
  • Raise error with loc

@cristianoc
Copy link
Contributor

I think the code is correct, and can be cleaned up later (in one place, the constraint is used to determine the type of props, an in a different place of the ppx, the already used constraint is now removed).
Just, for each such change a test file should be checked in.

@mununki
Copy link
Member Author

mununki commented Jul 21, 2022

I think the code is correct, and can be cleaned up later (in one place, the constraint is used to determine the type of props, an in a different place of the ppx, the already used constraint is now removed). Just, for each such change a test file should be checked in.

Sorry for making confusion.
Got it, I'll clean up. Thanks for catching the point.

@cristianoc
Copy link
Contributor

I think what's needed is a parsing test, see how the 2 cases in the example are parsed differently.

@mununki
Copy link
Member Author

mununki commented Jul 21, 2022

I think what's needed is a parsing test, see how the 2 cases in the example are parsed differently.

Got it, added.

@cristianoc
Copy link
Contributor

There's no reason the 2 examples should be treated differently. This looks like a parsing issue.
Though then V3, with the new compiler, should have the same problem. Does it?

@mununki
Copy link
Member Author

mununki commented Jul 21, 2022

Yes, it is. The difference between V3 and V4 for this case is the location of prop value. In V3, it is used as value for labeled argument, instead in V4, it is used as value for a field in record. In my guess, the parsing issue occurs due to where it is used.

@cristianoc
Copy link
Contributor

Not sure what's going on. Parsing is done once it reaches the ppx.
Will investigate later.

@cristianoc
Copy link
Contributor

OK so here's a real self-contained example, which I think was the intended example:

let optionMap = (x, f) =>
  switch x {
  | None => None
  | Some(v) => Some(f(v))
  }

module Parens = {
  @react.component
  let make = (~id=?) => {
    <div id=?{id->optionMap(x => x)} />
  }
}

@cristianoc
Copy link
Contributor

This compiles just fine with V4:
Screenshot 2022-07-21 at 18 40 40

@cristianoc
Copy link
Contributor

But one thing I have noticed. Check out this effect of having nominal types:

Screenshot 2022-07-21 at 18 42 03

@cristianoc
Copy link
Contributor

Going forward, I think it's more efficient to take a look directly at the project that does not compile, instead of trying to extract examples.

@cristianoc
Copy link
Contributor

@mattdamon108 thanks for the great work on all of this.
I think there are a couple more subtle issues to look into. Nearly there.

@mununki
Copy link
Member Author

mununki commented Jul 22, 2022

But one thing I have noticed. Check out this effect of having nominal types:

Screenshot 2022-07-21 at 18 42 03

Yes, it's nominal type effect. Expected usage might be this. 🤞
image

@mununki
Copy link
Member Author

mununki commented Jul 22, 2022

self-contained example

I think I didn't exactly understand what self-contained example stands for. Now I get it. You will get my self-contained example next time you need from me. 😄

@mununki
Copy link
Member Author

mununki commented Jul 22, 2022

@mattdamon108 thanks for the great work on all of this. I think there are a couple more subtle issues to look into. Nearly there.

It won't happen without your help and lead. You might remember my first suggestion was just the spread props support. 😆 Thank you for all of this.

@cristianoc
Copy link
Contributor

But one thing I have noticed. Check out this effect of having nominal types:
Screenshot 2022-07-21 at 18 42 03

Yes, it's nominal type effect. Expected usage might be this. 🤞 image

Because the type definition is implicit, one needs to be careful in documenting V4.
Also, that is one example that compiles in V3 and not V4. So one needs to document that it's going to happen.
We can worry about that later, once the last implementation issues are done.

@cristianoc
Copy link
Contributor

Better split up PRs in small ones. Will merge this.

@cristianoc cristianoc changed the title Fix backward compatibility of jsx ppx v4 Fix jsx V4 issue with type constraints and components names other than "make". Jul 22, 2022
@cristianoc cristianoc merged commit d24718d into master Jul 22, 2022
@cristianoc cristianoc deleted the fix-compat-jsx-ppx-v4 branch July 22, 2022 04:49
@mununki mununki mentioned this pull request Jul 22, 2022
11 tasks
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