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

fix(compiler): components typedef path aliases #4365

Merged
merged 14 commits into from
May 16, 2023
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 7 additions & 1 deletion src/compiler/types/generate-app-types.ts
Expand Up @@ -78,7 +78,13 @@ const generateComponentTypesFile = (config: d.Config, buildCtx: d.BuildCtx, areT
* data structure for each Stencil component in series, therefore the memory footprint of this entity will likely
* grow as more components (with additional types) are processed.
*/
typeImportData = updateReferenceTypeImports(typeImportData, allTypes, cmp, cmp.sourceFilePath);
typeImportData = updateReferenceTypeImports(
typeImportData,
allTypes,
cmp,
cmp.sourceFilePath,
config.tsCompilerOptions
);
if (cmp.events.length > 0) {
/**
* Only generate event detail types for components that have events.
Expand Down
101 changes: 101 additions & 0 deletions src/compiler/types/tests/generate-app-types.spec.ts
@@ -1,4 +1,5 @@
import { mockBuildCtx, mockCompilerCtx, mockValidatedConfig } from '@stencil/core/testing';
import ts, { Extension } from 'typescript';

import type * as d from '../../../declarations';
import { generateAppTypes } from '../generate-app-types';
Expand Down Expand Up @@ -1406,6 +1407,106 @@ declare module "@stencil/core" {
}
}
}
`,
{
immediateWrite: true,
}
);
});

it('should transform aliased paths', async () => {
// TODO(STENCIL-223): remove spy to test actual resolution behavior
jest.spyOn(ts, 'resolveModuleName').mockReturnValue({
resolvedModule: {
isExternalLibraryImport: false,
extension: Extension.Dts,
resolvedFileName: 'utils',
},
});
Copy link
Member

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 the rewrite-aliased-paths transformer I was able to get this to work by making sure I called patchTypescript, which then let me create files in the in-memory file system (see this setup function for instance:

async function pathTransformTranspile(component: string, inputFileName = 'module.tsx') {
const compilerContext: CompilerCtx = mockCompilerCtx();
const config = mockValidatedConfig();
patchTypescript(config, compilerContext.fs);
const mockPathsConfig: ts.CompilerOptions = {
paths: {
'@namespace': [path.join(config.rootDir, 'name/space.ts')],
'@namespace/subdir': [path.join(config.rootDir, 'name/space/subdir.ts')],
},
declaration: true,
};
// we need to have files in the `inMemoryFs` which TypeScript
// can resolve, otherwise it won't find the module and won't
// transform the module ID
await compilerContext.fs.writeFile(path.join(config.rootDir, 'name/space.ts'), 'export const foo = x => x');
await compilerContext.fs.writeFile(path.join(config.rootDir, 'name/space/subdir.ts'), 'export const bar = x => x;');
return transpileModule(
component,
null,
compilerContext,
[rewriteAliasedSourceFileImportPaths],
[],
[rewriteAliasedDTSImportPaths],
mockPathsConfig,
normalizePath(path.join(config.rootDir, inputFileName))
);
}
)

Any chance a similar approach would work here?

Copy link
Member Author

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.


const compilerComponentMeta = stubComponentCompilerMeta({
tagName: 'my-component',
componentClassName: 'MyComponent',
jsFilePath: '/some/stubbed/path/a/my-component.js',
sourceFilePath: '/some/stubbed/path/a/my-component.tsx',
sourceMapPath: '/some/stubbed/path/a/my-component.js.map',
hasProp: true,
properties: [
stubComponentCompilerProperty({
name: 'name',
complexType: {
original: 'UserImplementedPropType',
resolved: '"foo" | "bar"',
references: {
UserImplementedPropType: {
location: 'import',
path: '@utils',
},
},
},
}),
],
});
buildCtx.components = [compilerComponentMeta];
config.tsCompilerOptions = {};

await generateAppTypes(config, compilerCtx, buildCtx, 'src');

expect(mockFs.writeFile).toHaveBeenCalledWith(
'/components.d.ts',
`/* eslint-disable */
/* tslint:disable */
/**
* This is an autogenerated file created by the Stencil compiler.
* It contains typing information for all components that exist in this project.
*/
import { HTMLStencilElement, JSXBase } from "@stencil/core/internal";
import { UserImplementedPropType } from "utils";
export { UserImplementedPropType } from "utils";
export namespace Components {
/**
* docs
*/
interface MyComponent {
"name": UserImplementedPropType;
}
}
declare global {
/**
* docs
*/
interface HTMLMyComponentElement extends Components.MyComponent, HTMLStencilElement {
}
var HTMLMyComponentElement: {
prototype: HTMLMyComponentElement;
new (): HTMLMyComponentElement;
};
interface HTMLElementTagNameMap {
"my-component": HTMLMyComponentElement;
}
}
declare namespace LocalJSX {
/**
* docs
*/
interface MyComponent {
"name"?: UserImplementedPropType;
}
interface IntrinsicElements {
"my-component": MyComponent;
}
}
export { LocalJSX as JSX };
declare module "@stencil/core" {
export namespace JSX {
interface IntrinsicElements {
/**
* docs
*/
"my-component": LocalJSX.MyComponent & JSXBase.HTMLAttributes<HTMLMyComponentElement>;
}
}
}
`,
{
immediateWrite: true,
Expand Down
29 changes: 26 additions & 3 deletions src/compiler/types/update-import-refs.ts
@@ -1,4 +1,5 @@
import { dirname, resolve } from 'path';
import ts from 'typescript';

import type * as d from '../../declarations';

Expand All @@ -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(
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -83,6 +91,21 @@ const updateImportReferenceFactory = (typeCounts: Map<string, number>, filePath:
importResolvedFile = filePath;
} else if (typeReference.location === 'import') {
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(
Copy link
Member

Choose a reason for hiding this comment

The 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

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep. It should return

{
  resolvedModule: undefined;
}

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:

{
  resolvedModule: undefined;
  failedLookupLocations: _whateverShapeThisIs_[]
}

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)
);
Copy link
Member Author

Choose a reason for hiding this comment

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

Just FYI, this logic will currently transform the path aliases for the src/components.d.ts file we automatically generate at build time as well. This doesn't really cause any problems, but if we think that's confusing to users or just want to leave aliases in that file, I can update this logic a bit to ignore this block of code when writing to the src dir.

Aliases can exist there since it's not distributed code and the TS language server/any IDE would be able to handle the aliases in there.

Open to whatever the team thinks is best.


if (!resolvedModule.isExternalLibraryImport && resolvedModule.resolvedFileName) {
importResolvedFile = resolvedModule.resolvedFileName;
}
}
}

// If this is a relative path make it absolute
Expand Down