-
Notifications
You must be signed in to change notification settings - Fork 795
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
fix(types): components.d.ts type resolution for duplicate types #3337
Merged
Merged
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,18 +1,43 @@ | ||
import type * as d from '../../declarations'; | ||
import { getTextDocs } from '@utils'; | ||
import { updateTypeIdentifierNames } from './stencil-types'; | ||
|
||
/** | ||
* Generates the individual event types for all @Method() decorated events in a component | ||
* @param cmpMeta component runtime metadata for a single component | ||
* @param typeImportData locally/imported/globally used type names, which may be used to prevent naming collisions | ||
* @returns the generated type metadata | ||
*/ | ||
export const generateMethodTypes = (cmpMeta: d.ComponentCompilerMeta): d.TypeInfo => { | ||
export const generateMethodTypes = ( | ||
cmpMeta: d.ComponentCompilerMeta, | ||
typeImportData: d.TypesImportData | ||
): d.TypeInfo => { | ||
return cmpMeta.methods.map((cmpMethod) => ({ | ||
name: cmpMethod.name, | ||
type: cmpMethod.complexType.signature, | ||
type: getType(cmpMethod, typeImportData, cmpMeta.sourceFilePath), | ||
optional: false, | ||
required: false, | ||
internal: cmpMethod.internal, | ||
jsdoc: getTextDocs(cmpMethod.docs), | ||
})); | ||
}; | ||
|
||
/** | ||
* Determine the correct type name for all type(s) used by a class member annotated with `@Method()` | ||
* @param cmpMethod the compiler metadata for a single `@Method()` | ||
* @param typeImportData locally/imported/globally used type names, which may be used to prevent naming collisions | ||
* @param componentSourcePath the path to the component on disk | ||
* @returns the type associated with a `@Method()` | ||
*/ | ||
function getType( | ||
cmpMethod: d.ComponentCompilerMethod, | ||
typeImportData: d.TypesImportData, | ||
componentSourcePath: string | ||
): string { | ||
return updateTypeIdentifierNames( | ||
cmpMethod.complexType.references, | ||
typeImportData, | ||
componentSourcePath, | ||
cmpMethod.complexType.signature | ||
); | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
import * as d from '@stencil/core/declarations'; | ||
|
||
/** | ||
* Generates a stub {@link TypesImportData}. | ||
* @param overrides a partial implementation of `TypesImportData`. Any provided fields will override the defaults | ||
* provided by this function. | ||
* @returns the stubbed `TypesImportData` | ||
*/ | ||
export const stubTypesImportData = (overrides: Partial<d.TypesImportData> = {}): d.TypesImportData => { | ||
/** | ||
* By design, we do not provide any default values. the keys used in this data structure will be highly dependent on | ||
* the tests being written, and providing default values may lead to unexpected behavior when enumerating the returned | ||
* stub | ||
*/ | ||
const defaults: d.TypesImportData = {}; | ||
|
||
return { ...defaults, ...overrides }; | ||
}; |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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 afor ... 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.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.
A little, yes. Specifically, I'm concerned about the cost of these two lines in
updateTypeName
:where we build this regex and run
replace
globally oncurrentTypeName
. 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 likeO(m * n * p)
performance where:m
is the size ofComponentCompilerTypeReferences
n
is the size ofTypesImportData
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
andn
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).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.
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!