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

feat: Add disableAliases option #1576

Merged
merged 7 commits into from Apr 24, 2021
Merged

feat: Add disableAliases option #1576

merged 7 commits into from Apr 24, 2021

Conversation

Unnvaldr
Copy link
Contributor

@Unnvaldr Unnvaldr commented Apr 23, 2021

Resolves #1571

src/lib/converter/converter.ts Outdated Show resolved Hide resolved
src/lib/converter/symbols.ts Outdated Show resolved Hide resolved
@Unnvaldr Unnvaldr requested a review from Gerrit0 April 23, 2021 19:01
@Unnvaldr
Copy link
Contributor Author

Unnvaldr commented Apr 23, 2021

First post content was changed due to wrong issue linked

PS

Also workflow failed because of socket timeout

src/lib/converter/symbols.ts Outdated Show resolved Hide resolved
@Unnvaldr Unnvaldr requested a review from Gerrit0 April 24, 2021 16:59
Copy link
Collaborator

@Gerrit0 Gerrit0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@Gerrit0 Gerrit0 merged commit a446431 into TypeStrong:master Apr 24, 2021
@Unnvaldr Unnvaldr deleted the feat/alias-option branch April 24, 2021 21:04
@Unnvaldr
Copy link
Contributor Author

Unnvaldr commented Jun 7, 2021

@Gerrit0 I came back to this after a while and I've noticed strange extendedTypes behaviour.
While extended types reflections are properly resolved for a class (in my case alt-client.WorldObject), in alt-server.WorldObject they point still on first occurence in whole project (alt-client.WorldObject).
I tried to find where's the problem hiding, but so far I couldn't find logic behind assigning reflection ids to types created from classes heritageClauses.

edit: You can test it in this repo https://github.com/altmp/altv-types/tree/shared-pkg

@Gerrit0
Copy link
Collaborator

Gerrit0 commented Jun 8, 2021

Yeah, that doesn't surprise me. References are created with types having all their links resolved, so you'll end up with multiple reflections with the same symbol. The ImplementsPlugin relies on getting the symbol to perform inheritance analysis, and that hasn't been updated to support Symbol -> Reflection[] (which really needs to happen due to declaration merging anyways, but I haven't figured out a clean way to do it, and it doesn't break too many people)

@Unnvaldr
Copy link
Contributor Author

Unnvaldr commented Jun 8, 2021

Yeah, that doesn't surprise me. References are created with types having all their links resolved, so you'll end up with multiple reflections with the same symbol. The ImplementsPlugin relies on getting the symbol to perform inheritance analysis, and that hasn't been updated to support Symbol -> Reflection[] (which really needs to happen due to declaration merging anyways, but I haven't figured out a clean way to do it, and it doesn't break too many people)

Is there an issue created for this? If not, would be nice to have one.

@Unnvaldr
Copy link
Contributor Author

Unnvaldr commented Jun 8, 2021

Also on closer look, it seems that every reference from "shared" module is pointing to incorrect symbol in 2nd module, so it's even more vast that I originally assumed.

@Gerrit0
Copy link
Collaborator

Gerrit0 commented Jun 8, 2021

I don't think there's a canonical issue for it, there's probably some discussion in the 0.20 tracking thread, but I honestly don't remember... and yep, I kind of expected that for the latter as well.

Come to think of it, --packages might be a perfect fit for your repo since it will make TD create two programs and therefore the types will be completely separate... though whether that's a limitation of TD or a feature is kind of undefined right now.

@Unnvaldr
Copy link
Contributor Author

Unnvaldr commented Jun 9, 2021

In such case I think that we need to either properly document what happens if disableAliases is used or just entirely remove it.

@Gerrit0
Copy link
Collaborator

Gerrit0 commented Jun 9, 2021

Since it hasn't been published in a non-beta version (happy accident), I'm fine with just removing it - it does reveal more obviously problems with the architecture

Gerrit0 added a commit that referenced this pull request Jun 13, 2021
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.

Exports in ambient module don't get included in certain case
2 participants