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

Correctly handle generic model name resolution #427

Merged
merged 1 commit into from
Aug 19, 2019

Conversation

WoH
Copy link
Collaborator

@WoH WoH commented Aug 18, 2019

@WoH WoH mentioned this pull request Aug 18, 2019

const resolvedName = genericTypes.reduce((acc, generic) => {
if (ts.isTypeReferenceNode(generic) && generic.typeArguments && generic.typeArguments.length > 0) {
return [...acc, this.getTypeName(generic.typeName.getText(), generic.typeArguments)];
Copy link
Collaborator

Choose a reason for hiding this comment

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

@WoH I mentioned this in another recent PR, but there's a performance hit for using the spread operator since you are iterating through the acc array on every loop of the reduce. The implementation of the reduce could still be a pure function if it mutates the seed since the seed is not considered to be part of the inputs. In fact, it's much more standard in what I've seen to just use acc.push. Now I'm aware that genericTypes probably has a very small length, so the performance advantages are probably minimal, but I also think that .push is more readable. Here's the JSPerf from last time to demonstrate the performance advantages: https://jsperf.com/reduce-with-new-set-vs-seed

Copy link
Collaborator

Choose a reason for hiding this comment

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

Rather than have you make this acc.push update @WoH, I wanted to be considerate of the number of excellent contributions you've made. So I've made the update as part of #428 which fixes the other issue that Asterix presented to us. So I'll merge this PR so I can fold it into mine. Sound good? :)

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.

2 participants