Jsx ppx v4 part2

[cristianoc]

This moves #586 to a branch on the repo.

[cristianoc]

With V4 I get:

FAILED: src/ImportHookDefault.cmj

  We've found a bug for you!
  /Users/cristianocalcagno/GitHub/genType/examples/typescript-react-example/src/ImportHookDefault.res

  Multiple definition of the type name props.
  Names must be unique in a given structure or signature.

which is not surprising.

While this is a breaking change, I'm wondering whether it could be turned into a net positive.
If it make sense to enforce: at most one component for each module.
mmm that might be getting in the way of libraries or component factories etc

worth thinking about anyway

[cristianoc]

One possible view is that for component definitions, what the ppx does is completely transparent and documented.
So one can just write a component definition directly without even using the ppx.
That is at least an escape hatch that one could use.

So this is about just accepting the current situation and putting it in context.

A separate thing to think about is to tweak the name of the props type being defined.

[mununki]

One possible view is that for component definitions, what the ppx does is completely transparent and documented. So one can just write a component definition directly without even using the ppx. That is at least an escape hatch that one could use.

So this is about just accepting the current situation and putting it in context.

A separate thing to think about is to tweak the name of the props type being defined.

I had noticed it with the test case in innerModule before, I think it is worth thinking about: one component per module. If we tweak the name of the props type, the error will be gone in the definition site. In the caller site, the uppercase, the application site, is parsed only to Foo.make anyway. So, IMHO, I agree with your opinion that accepting the current situation and putting it in the context as you said.

[cristianoc]

Not sure I understand which one you're proposing:
1 rename "prop" where necessary to avoid multiple definitions
2 enforce one component per module (so many inner modules are ok)

Which one are you thinking about?

[mununki]

2 enforce one component per module (so many inner modules are ok)

This is what I'm proposing.

[cristianoc]

2 enforce one component per module (so many inner modules are ok)

This is what I'm proposing.

I think this needs to be enforced directly in the ppx. Not by a compiler error.

[cristianoc]

So for each module in the ast, check that at most 1 component definition is there.

[mununki]

Can I ask what your proposal is?

I think this needs to be enforced directly in the ppx. Not by a compiler error.

I guess it is not a difficult job to achieve with ast_iterator.

EDIT: not even ast_iterator is needed, I guess. We can identify how many react components are there at the module level.

[cristianoc]

Can I ask what your proposal is?

I think this needs to be enforced directly in the ppx. Not by a compiler error.

I guess it is not a difficult job to achieve with ast_iterator.

EDIT: not even ast_iterator is needed, I guess. We can identify how many react components are there at the module level.

Exactly. It fits naturally in the existing ppx mapper.

[cristianoc]

There are a lot of visual changes so don't know exactly everything that has changed.
But notice that hasReactComponent could be changed in a sub-module, and should be restore to its previous value when processing the submodule is finished.

[mununki]

There are a lot of visual changes so don't know exactly everything that has changed. But notice that hasReactComponent could be changed in a sub-module, and should be restore to its previous value when processing the submodule is finished.

Most diff is cased by indent by if else expression added. Actually, hasReactComponent value is restored per modules because it is assigned inside the structure and signature mapper which is the same level of saving and restoring config.

[mununki]

Shouldn't config.hasReactComponent be restored?

[mununki]

I added to restore the old value of config.hasReactComponent

[cristianoc]

This is looking good for an initial merge as WIP. Seems low-risk as existing projects are using V3 and should not be affected.