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): don't allow shadowRoot getter to avoid hydration issues #5912

Merged
merged 4 commits into from
Aug 1, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 48 additions & 0 deletions src/compiler/transformers/static-to-meta/component.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,14 @@ import { parseStringLiteral } from './string-literal';
import { parseStaticStyles } from './styles';
import { parseStaticWatchers } from './watchers';

const BLACKLISTED_COMPONENT_METHODS = [
/**
* If someone would define a getter called "shadowRoot" on a component
* this would cause issues when Stencil tries to hydrate the component.
*/
'shadowRoot',
];

/**
* Given a {@see ts.ClassDeclaration} which represents a Stencil component
* class declaration, parse and format various pieces of data about static class
Expand Down Expand Up @@ -148,6 +156,8 @@ export const parseStaticComponentMeta = (
};

const visitComponentChildNode = (node: ts.Node) => {
validateComponentMembers(node);

if (ts.isCallExpression(node)) {
parseCallExpression(cmp, node);
} else if (ts.isStringLiteral(node)) {
Expand Down Expand Up @@ -176,6 +186,44 @@ export const parseStaticComponentMeta = (
return cmpNode;
};

const validateComponentMembers = (node: ts.Node) => {
/**
* validate if:
*/
if (
/**
* the component has a getter called "shadowRoot"
*/
ts.isGetAccessorDeclaration(node) &&
ts.isIdentifier(node.name) &&
typeof node.name.escapedText === 'string' &&
BLACKLISTED_COMPONENT_METHODS.includes(node.name.escapedText) &&
/**
* the parent node is a class declaration
*/
ts.isClassDeclaration(node.parent)
) {
const propName = node.name.escapedText;
const decorator = ts.getDecorators(node.parent)[0];
/**
* the class is actually a Stencil component, has a decorator with a property named "tag"
*/
if (
ts.isCallExpression(decorator.expression) &&
decorator.expression.arguments.length === 1 &&
ts.isObjectLiteralExpression(decorator.expression.arguments[0]) &&
decorator.expression.arguments[0].properties.some(
(prop) => ts.isPropertyAssignment(prop) && prop.name.getText() === 'tag',
)
Comment on lines +212 to +217
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it easier to just check the decorator name?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, but you could potentially import it with any arbitrary name which is why I did this extra step of checking if the decorator has 1 argument which contains a tag property since this is the same everywhere.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah yeah, good point!

) {
const componentName = node.parent.name.getText();
throw new Error(
`The component "${componentName}" has a getter called "${propName}". This getter is reserved for use by Stencil components and should not be defined by the user.`,
);
}
}
};

const parseVirtualProps = (docs: d.CompilerJsDoc) => {
return docs.tags
.filter(({ name }) => name === 'virtualProp')
Expand Down
44 changes: 44 additions & 0 deletions src/compiler/transformers/test/parse-component.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,4 +23,48 @@ describe('parse component', () => {

expect(t.componentClassName).toBe('CmpA');
});

it('can not have shadowRoot getter', () => {
let error: Error | undefined;
try {
transpileModule(`
@Component({
tag: 'cmp-a'
})
export class CmpA {
get shadowRoot() {
return this;
}
}
`);
} catch (err: unknown) {
error = err as Error;
}

expect(error.message).toContain(
`The component "CmpA" has a getter called "shadowRoot". This getter is reserved for use by Stencil components and should not be defined by the user.`,
);
});

it('ignores shadowRoot getter in unrelated class', () => {
const t = transpileModule(`
@Component({
tag: 'cmp-a'
})
export class CmpA {
// use a better name for the getter
get elementShadowRoot() {
return this;
}
}

export class Unrelated {
get shadowRoot() {
return this;
}
}
`);

expect(t.componentClassName).toBe('CmpA');
});
});