-
-
Notifications
You must be signed in to change notification settings - Fork 737
feat: Add disableAliases option #1576
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
Conversation
First post content was changed due to wrong issue linked PS Also workflow failed because of socket timeout |
This reverts commit 53c7dee.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
@Gerrit0 I came back to this after a while and I've noticed strange extendedTypes behaviour. edit: You can test it in this repo https://github.com/altmp/altv-types/tree/shared-pkg |
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. |
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. |
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, |
In such case I think that we need to either properly document what happens if disableAliases is used or just entirely remove it. |
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 |
Resolves #1571