-
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(compiler): components typedef path aliases #4365
Changes from 6 commits
3aee6b7
1419307
790140e
7b10279
3844948
e24d34f
e461ab6
645dcda
555d189
7b4d47a
89347f4
637ad57
80fa940
864eccd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,5 @@ | ||
import { dirname, resolve } from 'path'; | ||
import ts from 'typescript'; | ||
|
||
import type * as d from '../../declarations'; | ||
|
||
|
@@ -8,15 +9,17 @@ import type * as d from '../../declarations'; | |
* @param typeCounts a map of seen types and the number of times the type has been seen | ||
* @param cmp the metadata associated with the component whose types are being inspected | ||
* @param filePath the path of the component file | ||
* @param tsOptions The TS compiler options to be used for resolving aliased module paths | ||
* @returns the updated import data | ||
*/ | ||
export const updateReferenceTypeImports = ( | ||
importDataObj: d.TypesImportData, | ||
typeCounts: Map<string, number>, | ||
cmp: d.ComponentCompilerMeta, | ||
filePath: string | ||
filePath: string, | ||
tsOptions: ts.CompilerOptions | ||
): d.TypesImportData => { | ||
const updateImportReferences = updateImportReferenceFactory(typeCounts, filePath); | ||
const updateImportReferences = updateImportReferenceFactory(typeCounts, filePath, tsOptions); | ||
|
||
return [...cmp.properties, ...cmp.events, ...cmp.methods] | ||
.filter( | ||
|
@@ -44,9 +47,14 @@ type ImportReferenceUpdater = ( | |
* Factory function to create an `ImportReferenceUpdater` instance | ||
* @param typeCounts a key-value store of seen type names and the number of times the type name has been seen | ||
* @param filePath the path of the file containing the component whose imports are being inspected | ||
* @param tsOptions The TS compiler options to be used for resolving aliased module paths | ||
* @returns an `ImportReferenceUpdater` instance for updating import references in the provided `filePath` | ||
*/ | ||
const updateImportReferenceFactory = (typeCounts: Map<string, number>, filePath: string): ImportReferenceUpdater => { | ||
const updateImportReferenceFactory = ( | ||
typeCounts: Map<string, number>, | ||
filePath: string, | ||
tsOptions: ts.CompilerOptions | ||
): ImportReferenceUpdater => { | ||
/** | ||
* Determines the number of times that a type identifier (name) has been used. If an identifier has been used before, | ||
* append the number of times the identifier has been seen to its name to avoid future naming collisions | ||
|
@@ -81,8 +89,23 @@ const updateImportReferenceFactory = (typeCounts: Map<string, number>, filePath: | |
// If local then import location is the current file | ||
} else if (typeReference.location === 'local') { | ||
importResolvedFile = filePath; | ||
} else if (typeReference.location === 'import') { | ||
importResolvedFile = typeReference.path; | ||
} else { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe it's okay to remove the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yah - I think this is OK as well. |
||
importResolvedFile = typeReference.path!; | ||
|
||
// We only care to resolve any _potential_ aliased | ||
// modules if we're not already certain the path isn't an alias | ||
if (!importResolvedFile.startsWith('.')) { | ||
const { resolvedModule } = ts.resolveModuleName( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the event that module resolution fails, does this TS API just return a falsy value? I'm looking at its implementation now, but wanted to double check my understanding There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep. It should return
But, it's also supposed to return the failed lookup locations. Doesn't look like this is included in the typedefs for whatever reason, but I believe the type would be:
I have seen the API return the lookup locations, so not sure why it's not on the object, but it should be there if we wanted those for some reason |
||
typeReference.path!, | ||
filePath, | ||
tsOptions, | ||
ts.createCompilerHost(tsOptions) | ||
); | ||
|
||
if (resolvedModule && !resolvedModule.isExternalLibraryImport && resolvedModule.resolvedFileName) { | ||
importResolvedFile = resolvedModule.resolvedFileName; | ||
} | ||
} | ||
} | ||
|
||
// If this is a relative path make it absolute | ||
|
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.
so for now the test isn't testing that the values in
paths
are used - I assume this was the stuff you had trouble with testing? when testing therewrite-aliased-paths
transformer I was able to get this to work by making sure I calledpatchTypescript
, which then let me create files in the in-memory file system (see this setup function for instance:stencil/src/compiler/transformers/test/rewrite-aliased-paths.spec.ts
Lines 21 to 51 in 39ab8d9
Any chance a similar approach would work here?
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.
Ooh nifty! I'll check this out today.