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.

Sign up for GitHub

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

Jump to bottom

fix(test): fix infinite loops w/ react and @testing-library/dom #4188

Merged
merged 1 commit into from
Mar 24, 2023
Merged

fix(test): fix infinite loops w/ react and @testing-library/dom #4188

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
21 changes: 12 additions & 9 deletions src/compiler/transformers/add-component-meta-proxy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,24 +47,27 @@ export const createComponentMetadataProxy = (compilerMeta: d.ComponentCompilerMe
};

/**
* Create a call expression for wrapping a component represented as an anonymous class in a proxy. This call expression
* takes a form:
* Create a call expression for wrapping a component represented as a class
* expression in a proxy. This call expression takes the form:
*
* ```ts
* PROXY_CUSTOM_ELEMENT(Clazz, Metadata);
* ```
*
* where
* - `PROXY_CUSTOM_ELEMENT` is a Stencil internal identifier that will be replaced with the name of the actual function
* name at compile name
* - `Clazz` is an anonymous class to be proxied
* - `PROXY_CUSTOM_ELEMENT` is a Stencil internal identifier that will be
* replaced with the name of the actual function name at compile name
* - `Clazz` is a class expression to be proxied
* - `Metadata` is the compiler metadata associated with the Stencil component
*
* @param compilerMeta compiler metadata associated with the component to be wrapped in a proxy
* @param clazz the anonymous class to proxy
* @param compilerMeta compiler metadata associated with the component to be
* wrapped in a proxy
* @param clazz the class expression to proxy
* @returns the generated call expression
*/
export const createAnonymousClassMetadataProxy = (
export const createClassMetadataProxy = (
compilerMeta: d.ComponentCompilerMeta,
clazz: ts.Expression
clazz: ts.ClassExpression
Copy link
Member

Choose a reason for hiding this comment

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

"clazz" 😂

Copy link
Member

Choose a reason for hiding this comment

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

FWIW ☕ - https://stackoverflow.com/questions/2529974/why-do-java-programmers-like-to-name-a-variable-clazz

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in python I'm using to seeing klass for the same reason haha

): ts.CallExpression => {
const compactMeta: d.ComponentRuntimeMetaCompact = formatComponentRuntimeMeta(compilerMeta, true);
const literalMeta = convertValueToLiteral(compactMeta);
Expand Down
18 changes: 16 additions & 2 deletions src/compiler/transformers/component-native/proxy-custom-element-function.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import ts from 'typescript';

import type * as d from '../../../declarations';
import { createAnonymousClassMetadataProxy } from '../add-component-meta-proxy';
import { createClassMetadataProxy } from '../add-component-meta-proxy';
import { addImports } from '../add-imports';
import { RUNTIME_APIS } from '../core-runtime-apis';
import { getModuleFromSourceFile } from '../transform-utils';
Expand Down Expand Up @@ -47,8 +47,22 @@ export const proxyCustomElement = (
continue;
}

// to narrow the type of `declaration.initializer` to `ts.ClassExpression`
if (!ts.isClassExpression(declaration.initializer)) {
continue;
}
Comment on lines +50 to +53
Copy link
Member

Choose a reason for hiding this comment

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

This feels a little sketchy to me. Is there a different way to do this type-cast/assertion?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could instead do if (ts.isClassExpresion(declaration.initializer)) { and then put all the code within the if block - that essentially amounts to the same thing though, just changing a lot more lines. I don't think typically we would get to this point and have declaration.initializer not be the thing we want but I thought there's no harm in a little more type safety here. What's your concern exactly?

Copy link
Member

Choose a reason for hiding this comment

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

Not necessarily concerned, this just stood out to me as a pattern I haven't really seen. I've always strayed away from using continue. I've not too worried if you and @rwaskiewicz are cool with it

Copy link
Member

Choose a reason for hiding this comment

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

I'm OK with this as-is. Using an if statement with a continue achieves the same thing as if (ts.isClassExpresion(declaration.initializer)) {, just with less nesting. I don't think there's a better (safer) way to narrow the type here. I (personally) prefer less nesting, if only for less lines of code changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah I'm also in favor of less nesting where we can, and I also generally favor the 'should we be here? no? ok lets get out right now' sort of early-return pattern (well, early continue pattern in this case)


const renamedClassExpression = ts.factory.updateClassExpression(
declaration.initializer,
ts.getModifiers(declaration.initializer),
ts.factory.createIdentifier(principalComponent.componentClassName),
declaration.initializer.typeParameters,
declaration.initializer.heritageClauses,
declaration.initializer.members
);

// wrap the Stencil component's class declaration in a component proxy
const proxyCreationCall = createAnonymousClassMetadataProxy(principalComponent, declaration.initializer);
const proxyCreationCall = createClassMetadataProxy(principalComponent, renamedClassExpression);
ts.addSyntheticLeadingComment(proxyCreationCall, ts.SyntaxKind.MultiLineCommentTrivia, '@__PURE__', false);

// update the component's variable declaration to use the new initializer
Expand Down
14 changes: 7 additions & 7 deletions src/compiler/transformers/test/add-component-meta-proxy.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,12 @@ import ts from 'typescript';
import { stubComponentCompilerMeta } from '../../../compiler/types/tests/ComponentCompilerMeta.stub';
import type * as d from '../../../declarations';
import * as FormatComponentRuntimeMeta from '../../../utils/format-component-runtime-meta';
import { createAnonymousClassMetadataProxy } from '../add-component-meta-proxy';
import { createClassMetadataProxy } from '../add-component-meta-proxy';
import { HTML_ELEMENT } from '../core-runtime-apis';
import * as TransformUtils from '../transform-utils';

describe('add-component-meta-proxy', () => {
describe('createAnonymousClassMetadataProxy()', () => {
describe('createClassMetadataProxy()', () => {
let classExpr: ts.ClassExpression;
let htmlElementHeritageClause: ts.HeritageClause;
let literalMetadata: ts.StringLiteral;
Expand Down Expand Up @@ -51,33 +51,33 @@ describe('add-component-meta-proxy', () => {
});

it('returns a call expression', () => {
const result: ts.CallExpression = createAnonymousClassMetadataProxy(stubComponentCompilerMeta(), classExpr);
const result: ts.CallExpression = createClassMetadataProxy(stubComponentCompilerMeta(), classExpr);

expect(ts.isCallExpression(result)).toBe(true);
});

it('wraps the initializer in PROXY_CUSTOM_ELEMENT', () => {
const result: ts.CallExpression = createAnonymousClassMetadataProxy(stubComponentCompilerMeta(), classExpr);
const result: ts.CallExpression = createClassMetadataProxy(stubComponentCompilerMeta(), classExpr);

expect((result.expression as ts.Identifier).escapedText).toBe('___stencil_proxyCustomElement');
});

it("doesn't add any type arguments to the call", () => {
const result: ts.CallExpression = createAnonymousClassMetadataProxy(stubComponentCompilerMeta(), classExpr);
const result: ts.CallExpression = createClassMetadataProxy(stubComponentCompilerMeta(), classExpr);

expect(result.typeArguments).toHaveLength(0);
});

it('adds the correct arguments to the PROXY_CUSTOM_ELEMENT call', () => {
const result: ts.CallExpression = createAnonymousClassMetadataProxy(stubComponentCompilerMeta(), classExpr);
const result: ts.CallExpression = createClassMetadataProxy(stubComponentCompilerMeta(), classExpr);

expect(result.arguments).toHaveLength(2);
expect(result.arguments[0]).toBe(classExpr);
expect(result.arguments[1]).toBe(literalMetadata);
});

it('includes the heritage clause', () => {
const result: ts.CallExpression = createAnonymousClassMetadataProxy(stubComponentCompilerMeta(), classExpr);
const result: ts.CallExpression = createClassMetadataProxy(stubComponentCompilerMeta(), classExpr);

expect(result.arguments.length).toBeGreaterThanOrEqual(1);
const createdClassExpression = result.arguments[0];
Expand Down
33 changes: 16 additions & 17 deletions src/compiler/transformers/test/proxy-custom-element-function.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,9 @@ describe('proxy-custom-element-function', () => {
ReturnType<typeof TransformUtils.getModuleFromSourceFile>,
Parameters<typeof TransformUtils.getModuleFromSourceFile>
>;
let createAnonymousClassMetadataProxySpy: jest.SpyInstance<
ReturnType<typeof AddComponentMetaProxy.createAnonymousClassMetadataProxy>,
Parameters<typeof AddComponentMetaProxy.createAnonymousClassMetadataProxy>
let createClassMetadataProxySpy: jest.SpyInstance<
ReturnType<typeof AddComponentMetaProxy.createClassMetadataProxy>,
Parameters<typeof AddComponentMetaProxy.createClassMetadataProxy>
>;

beforeEach(() => {
Expand Down Expand Up @@ -47,20 +47,19 @@ describe('proxy-custom-element-function', () => {
} as d.Module;
});

createAnonymousClassMetadataProxySpy = jest.spyOn(AddComponentMetaProxy, 'createAnonymousClassMetadataProxy');
createAnonymousClassMetadataProxySpy.mockImplementation(
(_compilerMeta: d.ComponentCompilerMeta, clazz: ts.Expression) =>
ts.factory.createCallExpression(
ts.factory.createIdentifier(PROXY_CUSTOM_ELEMENT),
[],
[clazz, ts.factory.createTrue()]
)
createClassMetadataProxySpy = jest.spyOn(AddComponentMetaProxy, 'createClassMetadataProxy');
createClassMetadataProxySpy.mockImplementation((_compilerMeta: d.ComponentCompilerMeta, clazz: ts.Expression) =>
ts.factory.createCallExpression(
ts.factory.createIdentifier(PROXY_CUSTOM_ELEMENT),
[],
[clazz, ts.factory.createTrue()]
)
);
});

afterEach(() => {
getModuleFromSourceFileSpy.mockRestore();
createAnonymousClassMetadataProxySpy.mockRestore();
createClassMetadataProxySpy.mockRestore();
});

describe('proxyCustomElement()', () => {
Expand All @@ -82,7 +81,7 @@ describe('proxy-custom-element-function', () => {
const transpiledModule = transpileModule(code, null, compilerCtx, [], [transformer]);

expect(transpiledModule.outputText).toContain(
`export const ${componentClassName} = /*@__PURE__*/ __stencil_proxyCustomElement(class extends HTMLElement {}, true);`
`export const ${componentClassName} = /*@__PURE__*/ __stencil_proxyCustomElement(class ${componentClassName} extends HTMLElement {}, true);`
);
});

Expand All @@ -94,7 +93,7 @@ describe('proxy-custom-element-function', () => {
const transpiledModule = transpileModule(code, null, compilerCtx, [], [transformer]);

expect(transpiledModule.outputText).toContain(
`export const foo = 'hello world!', ${componentClassName} = /*@__PURE__*/ __stencil_proxyCustomElement(class extends HTMLElement {}, true);`
`export const foo = 'hello world!', ${componentClassName} = /*@__PURE__*/ __stencil_proxyCustomElement(class ${componentClassName} extends HTMLElement {}, true);`
);
});

Expand All @@ -105,18 +104,18 @@ describe('proxy-custom-element-function', () => {
const transpiledModule = transpileModule(code, null, compilerCtx, [], [transformer]);

expect(transpiledModule.outputText).toContain(
`export const ${componentClassName} = /*@__PURE__*/ __stencil_proxyCustomElement(class extends HTMLElement {}, true), foo = 'hello world!';`
`export const ${componentClassName} = /*@__PURE__*/ __stencil_proxyCustomElement(class ${componentClassName} extends HTMLElement {}, true), foo = 'hello world!';`
);
});

it('wraps a class initializer properly in the middle of multiple variable declarations', () => {
const code = `const foo = 'hello world!', ${componentClassName} = class extends HTMLElement {}, bar = 'goodbye?'`;
const code = `const foo = 'hello world!', ${componentClassName} = class ${componentClassName} extends HTMLElement {}, bar = 'goodbye?'`;

const transformer = proxyCustomElement(compilerCtx, transformOpts);
const transpiledModule = transpileModule(code, null, compilerCtx, [], [transformer]);

expect(transpiledModule.outputText).toContain(
`export const foo = 'hello world!', ${componentClassName} = /*@__PURE__*/ __stencil_proxyCustomElement(class extends HTMLElement {}, true), bar = 'goodbye?';`
`export const foo = 'hello world!', ${componentClassName} = /*@__PURE__*/ __stencil_proxyCustomElement(class ${componentClassName} extends HTMLElement {}, true), bar = 'goodbye?';`
);
});
});
Expand Down