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

Different Array<T> and Promise<T> types getting assigned the same metadata store id #33

Closed
bjsi opened this issue Mar 4, 2022 · 7 comments
Assignees
Labels
bug Something isn't working fixed Fix has been merged for this issue

Comments

@bjsi
Copy link

bjsi commented Mar 4, 2022

I ran into an issue with using getType on arrays and promises.

const x = getType<string[]>().getTypeArguments()[0]; // returns string
const y  = getType<number[]>().getTypeArguments()[0] // also returns string

I cloned the library and ran in debug mode inside the test suite:

image

I think the problem is that string[] and number[] are getting assigned the same metadata store id.

Let me know if I can help in any way, maybe this is already fixed in the update you are working on.

@Hookyns
Copy link
Owner

Hookyns commented Mar 4, 2022

I think I know what is going on.
You can try to fix, if you want. 🙂 I'm off for today...

The arrays are the same ts.Symbol probably so they have the same id.

export function getTypeId(type: ts.Type, typeChecker: ts.TypeChecker): number | undefined
{
const symbol = getTypeSymbol(type, typeChecker);
if (symbol == undefined)
{
return;
}
return getSymbolId(symbol);
}

In the new version I use type.id, not symbol.id, it should fix this issue, but it may cause other issues... Not every symbol has id but every type has. Only types with Id are stored, others are inlined... but it may not be an issue, but it will generate bigger metadata.

@bjsi
Copy link
Author

bjsi commented Mar 5, 2022

Thanks, I'll play around and see if I can fix.

@Hookyns
Copy link
Owner

Hookyns commented Mar 7, 2022

I'm working on it.

Return type's id instead of symbol's id is not enough. I've made several other changes to make it work, everything seems alright but two tests are failing. Those tests seems unrelated to this but idk there must be something,.. I'm trying to solve that.

@Hookyns
Copy link
Owner

Hookyns commented Mar 7, 2022

Solved.

I've published tst-reflect-transformer@0.9.5.

@Hookyns Hookyns self-assigned this Mar 7, 2022
@Hookyns Hookyns added bug Something isn't working fixed Fix has been merged for this issue labels Mar 7, 2022
@bjsi
Copy link
Author

bjsi commented Mar 7, 2022

Awesome, looks like the issue is fixed for me!

@bjsi bjsi closed this as completed Mar 7, 2022
@Hookyns
Copy link
Owner

Hookyns commented Aug 29, 2022

@all-contributors please add @bjsi for bug report

@allcontributors
Copy link
Contributor

@Hookyns

I've put up a pull request to add @bjsi! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working fixed Fix has been merged for this issue
Projects
None yet
Development

No branches or pull requests

2 participants