fix(types): components.d.ts type resolution for duplicate types

[rwaskiewicz]

Pull request checklist

Please check if your PR fulfills the following requirements:

• Tests for the changes have been added (for bug fixes / features)
• Docs have been reviewed and added / updated if needed (for bug fixes / features)
• Build (npm run build) was run locally and any changes were pushed
• Unit tests (npm test) were run locally and passed
• E2E Tests (npm run test.karma.prod) were run locally and passed
• Prettier (npm run prettier) was run locally and passed

Pull request type

Please check the type of change your PR introduces:

• Bugfix
• Feature
• Refactoring (no functional changes, no api changes)
• Build related changes
• Documentation content changes
• Other (please describe):

What is the current behavior?

See the linked GH Issue below

GitHub Issue Number: #2859

What is the new behavior?

this commit updates the type resolution of types used by components when
generating a components.d.ts file. types that have been deduplicated
are now piped to functions used to generate the types for class members
that are decorated with @Prop(), @Event(), and @Method(), and used
when generating the types for each.

Does this introduce a breaking change?

• Yes
• No

Testing

Unit tests have been written + passed

Manually we can verify this against the reproduction repo:

First, let's get the baseline setup:

git clone https://github.com/jcfranco/stencil-duplicate-types-not-assigned-correctly-in-component-typings sample
cd sample
npm i && git commit -am 'baseline after npm install'
npm i @stencil/core@latest
git commit -am 'install latest stencil version, 2.15.0'
npm run build
git commit -am 'build with stencil v2.15.0'
git add *
git commit -m 'build with stencil v2.15.0'

In the generated src/components.d.ts file, we can confirm the bug:

import { Name } from "./components/my-person/resources";
import { Name as Name1 } from "./components/my-pupper/resources";
export namespace Components {
    interface MyComponent {
        "saySomething": (name: Name) => Promise<void>;
    }
    interface MyPupper {
        "saySomething": (name: Name) => Promise<void>;
    }
}

Note how MyPupper's saySomething method uses Name, not Name1.

By checking out this branch, running npm ci && npm build && npm pack, we can generated a tarball with the changes included in this PR, we can install the tarball in the reproduction repo + rebuild

npm i <PATH_TO_TAR>
npm run build

which generates:

import { Name } from "./components/my-person/resources";
import { Name as Name1 } from "./components/my-pupper/resources";
export namespace Components {
    interface MyComponent {
        "saySomething": (name: Name) => Promise<void>;
    }
    interface MyPupper {
        "saySomething": (name: Name1) => Promise<void>;
    }
}

Other information

[alicewriteswrongs]

just tested this out and was able to 1) repro the issue with the repo provided and 2) confirm that this PR does fix it. reading through the code now!

[alicewriteswrongs]

I think that looking through everything pretty much looks good, I noticed like just a few really small things that we might want to change but otherwise things look relatively straightforward.

One thing that I want to do is sort of check my understanding of what's going on here to make sure I'm getting what we're changing / how we're fixing the issue with duplicate type names.

So here's my understanding of where this change intervenes in the Stencil compilation process and what we're doing.

What's the problem?

When we're compiling a stencil project one of the things we output is a components.d.ts file which includes type declarations for all the components in the project.¹

Since this file contains type information pulled from all of our separate component source files, which can be thought of as separate type namespaces or contexts, there is a possibility of a name clash if:

1. two different modules import different types from elsewhere and either one renames the import to clash with the name in the other module, they both rename the import to the same thing, or something along those lines
2. two modules each export a type with the same name which are each in turn imported in (separate) component files².

If we hit either of these scenarios it will lead to a situation in which we're referencing the wrong type. I don't think this will have a runtime effect but any typescript project which is importing the components and trying to use them will have errors.

How do we fix it?

So the way to fix this is that when we are merging together the type information from our separate component source files we need to keep track of the global imports and type references in order to check that a name that we're going to use in a particular component's portion of the components.d.ts file has not already been used by another component. If it has been, we need to do something like Name -> Name1 to ensure that the second component's type declaration does not erroneously reference a type that was pulled in to components.d.ts by another component.

Making this work requires edits to the function responsible for getting type information for a component (generateComponentTypes) and the sub-functions to which it delegates (generatePropTypes, generateEventTypes, and so on). The edit we need to make to each of these little functions is pretty much the same - they all need to be able to look at the TypesImportData object that we update w/ everything we're going to be importing into components.d.ts in order to see if the name of a type imported by the component in question needs to be updated to not clash with other imports.

This work is done in the updateTypeIdentifierNames function based on information already returned from the updateReferenceTypeImports function.

updateReferenceTypeImports basically puts together a map from filepaths (of source files) to an array of all the types that are found within them. The objects in that array (of type TypesMemberNameData) have two properties, localName and importName. Local name is what they are called in their home modules, and importName is a disambiguated name (like Name1 in the repro example) that will be safe to import in our mushed-together components.d.ts namespace.

updateReferenceTypeImports will basically take that TypesImportData object that we get back from updateReferenceTypeImports and the ComponentCompilerTypeReferences for a given component. Then it looks through the reference types and, for each one:

1. figures out what file path the reference is from
2. looks at TypesImportData in order to get a new, components.d.ts namespace-safe name for the reference in question, since TypesImportData is already set up that way.
3. Create a regular expression to update imports for a possibly-clashing type to a non-clashing type.

Then of course we run those regexes and the imports get fixed. Then all this stuff gets returned back to the generateComponentTypesFile in the same way it did before this change.

Anyway — sorry for the essay! I was just thinking that it's hard to review something like this if I don't know what's going on, so I wanted to really make sure I understand what we're trying to do, and how this fixes the issue we are seeing. Let me know where I'm missing things, or if there's just any other context that would be helpful for understanding what's going on here.

Footnotes

1. It's a secondary question w/r/t this PR but I'm not clear on why we need to generate this file ourselves and why TypeScript can't do so. I feel like I get what we're doing but some part of the why I haven't discovered yet. ↩

2. I believe this was the scenario in the reproduction repo. ↩

[alicewriteswrongs]

should importResolvedFile be undefined | string here? I just noticed that ComponentCompilerTypeReference.path is typed with ?

[rwaskiewicz]

Ah, good catch. Fixed in bba3054

[alicewriteswrongs]

this specfile is very helpful for understanding what this is all doing

[rwaskiewicz]

🎉 🎉 🎉

[alicewriteswrongs]

one thing I'm wondering - this function is going to be called for every component prop, every event, and so on, and then for each of those we're doing two nested for loops. Are we worried about the performance impact of doing that? Do you think it's worthwhile / necessary to do a little measurement to see if there's an impact? When I see a for ... of within a for ... of and then realize that this function is itself called within a loop I wonder, but I don't feel like I have enough context to know how big the things we're looping through are going to be. So possibly not a concern, but wanted to flag it to see your thoughts and make sure I'm understanding what's going on.

[rwaskiewicz]

Are we worried about the performance impact of doing that?

A little, yes. Specifically, I'm concerned about the cost of these two lines in updateTypeName:

  const typeNameRegex = new RegExp(`${typeAlias.localName}\\b${endingStrChar}`, 'g');
  return currentTypeName.replace(typeNameRegex, typeAlias.importName);

where we build this regex and run replace globally on currentTypeName. I tried to add as many cases where we can avoid this work as possible (if there's no alias to replace a collision with, if we don't see any collisions at all, etc.), but we still see something like O(m * n * p) performance where:

• m is the size of ComponentCompilerTypeReferences
• n is the size of TypesImportData
• p is the length of the string type of the collision (maybe, the perf implications may be worse here with allocating memory for a new string)

My hope is the m and n are relatively small since we evaluate this on a per component (and therefore per file) basis. p is a bit trickier to optimize ATM, and is going to require some rework that I planned in STENCIL-418+STENCIL-419 (we'd need to move some of this cost to allocating/storing the actual types instead of flattening them to strings).

[alicewriteswrongs]

Ok that all makes sense, I was thinking that given that we're going through a component at a time it shouldn't be a huge issue, but if stencil users have some enormous components with 30 or 40 imports we might see some real impact there.

Incidentally, this seems like exactly the type of situation where some automated performance testing against a large stencil project would be super handy!

[rwaskiewicz]

@alicewriteswrongs

You're understanding of what's going on here is spot on. No worries for the essay! 😆

One thing that's probably worth clarifying:

Since this file contains type information pulled from all of our separate component source files, which can be thought of as separate type namespaces or contexts, there is a possibility of a name clash if:

1. two different modules import different types from elsewhere and either one renames the import to clash with the name in the other module, they both rename the import to the same thing, or something along those lines

This is correct, although Stencil doesn't handle aliasing in import statements correctly 100% of the time. I added a reproduction case/bug to the backlog as STENCIL-429 for us to work through cases where import { SomeType as YetAnotherType } from './my-types'; isn't always handled correctly

[alicewriteswrongs]

The thought occurred to me, what if in ComponentA.tsx I do import { Name as MyName } from './libA' and then in ComponentB.tsx I do import { Name as MyName } from './libB', since it looked like the code was not set up to necessarily handle that sort of collision. Well, something to fix in the future I guess!

[alicewriteswrongs]

only really have one take-it-or-leave-it comment, other than that I think everything looks good!

[alicewriteswrongs]

sort of a minor thing, but it might be slightly more readable to move this function earlier in the block, up above where it's used, so the reader is encouraged to at least glance at it before reading the tests (I was thinking the same thing for testTypeIsTransformed below)

[rwaskiewicz]

I think it could go either way - I generally prefer to order functions in the order they're called, but that's more of a subjective opinion (at least in this scenario in JavaScript where order doesn't affect the execution). My vote for now would be to leave it as it for now. In the future I'd like to get ESLint set up in the codebase, and then we can let it warn us when we violate rules (we'll still have to decide/agree on the rules we run though 😉)

[alicewriteswrongs]

I did check earlier today out of curiousity and adding eslint w/ basic rules doesn't produce that many errors:

most of them are 'hey you used any, don't do that!'