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

Fix @genType issue with module aliases #6113

Merged
merged 7 commits into from
Apr 9, 2023
Merged

Conversation

cristianoc
Copy link
Collaborator

@cristianoc cristianoc commented Apr 6, 2023

See #6112
When module aliases module A = B are used, the aliasing information is lost if it happens in another file. There's enough logic to adapt the type export in the presence of module aliases. However this works locally, but not in a file that has no type exports and only a module alias.

This PR:

  • allows @genType at the leve of modules
  • expands B in module A = B

@cristianoc cristianoc changed the title WIP: Demonstrate @genType issue with module aliasesgi WIP: Demonstrate @genType issue with module aliases Apr 6, 2023
@cristianoc
Copy link
Collaborator Author

There are a couple of ways one could try to handle this:

  1. special-case module aliases, and go past them (consistent with e.g. jump to location in the editor)
  2. allow module-level @genType annotations and add one to the module alias

@cristianoc
Copy link
Collaborator Author

cristianoc commented Apr 8, 2023

@jmagaram would you take a look at this? Notice I changed Wrapper.res to add an include:

@genType
module MyModuleAlias = {include MyModule}

This should take care of your examples, but still worth double checking.

If this works, I will separately look into making in unnecessary to use the include, which can be expensive.

@jmagaram
Copy link
Contributor

jmagaram commented Apr 8, 2023 via email

@cristianoc
Copy link
Collaborator Author

Yes. I think if you look at my comments in the original issue you can see I suggested that as a possible solution. It is essentially rewriting the whole module in a different place.

Great.
We don't want to rely on include, so next I'll look into how to remove it.

when a module alias is created

```res
module A = B
```

expand the definition of B (as in an include) for genType's purposes
@cristianoc cristianoc changed the title WIP: Demonstrate @genType issue with module aliases Fix @genType issue with module aliases Apr 9, 2023
@cristianoc
Copy link
Collaborator Author

@jmagaram take a look now. This is ready for another set of testing.
While the example discussed seems to work, it does not hurt to try a few variations. And also watch for any unintended consequences.

Base on this PR, the way to go is to annotate the aliasing:

@genType
module A = B

@cristianoc cristianoc merged commit 9c378e8 into master Apr 9, 2023
14 checks passed
@cristianoc cristianoc deleted the gentype-module-alias-issue branch April 9, 2023 08:15
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

Successfully merging this pull request may close these issues.

None yet

2 participants