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

fix(compiler): make style getter behavior consistent between default and native components #5144

Merged
merged 3 commits into from
Dec 11, 2023
Merged

fix(compiler): make style getter behavior consistent between default and native components #5144

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4159e2e
fix(compiler): make style getter behavior consistent between default …
christian-bromann Dec 8, 2023
4bf5be3
apply changes for multi mode styles
christian-bromann Dec 8, 2023
c65c6e8
add comments that order of ifs must match
christian-bromann Dec 11, 2023
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
26 changes: 19 additions & 7 deletions src/compiler/transformers/add-static-style.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,33 +74,45 @@ const getMultipleModeStyle = (
const styleModes: ts.ObjectLiteralElementLike[] = [];

styles.forEach((style) => {
/**
* the order of these if statements must match with
* - {@link src/compiler/transformers/component-native/native-static-style.ts#addSingleStyleGetter}
* - {@link src/compiler/transformers/component-native/native-static-style.ts#addMultipleModeStyleGetter}
* - {@link src/compiler/transformers/add-static-style.ts#getMultipleModeStyle}
*/
if (typeof style.styleStr === 'string') {
// inline the style string
// static get style() { return { ios: "string" }; }
const styleLiteral = createStyleLiteral(cmp, style, commentOriginalSelector);
const propStr = ts.factory.createPropertyAssignment(style.modeName, styleLiteral);
styleModes.push(propStr);
} else if (typeof style.styleIdentifier === 'string') {
// direct import already written in the source code
// import myTagIosStyle from './import-path.css';
// static get style() { return { ios: myTagIosStyle }; }
const styleIdentifier = ts.factory.createIdentifier(style.styleIdentifier);
const propIdentifier = ts.factory.createPropertyAssignment(style.modeName, styleIdentifier);
styleModes.push(propIdentifier);
} else if (Array.isArray(style.externalStyles) && style.externalStyles.length > 0) {
// import generated from @Component() styleUrls option
// import myTagIosStyle from './import-path.css';
// static get style() { return { ios: myTagIosStyle }; }
const styleUrlIdentifier = createStyleIdentifier(cmp, style);
const propUrlIdentifier = ts.factory.createPropertyAssignment(style.modeName, styleUrlIdentifier);
styleModes.push(propUrlIdentifier);
} else if (typeof style.styleIdentifier === 'string') {
// direct import already written in the source code
// import myTagIosStyle from './import-path.css';
// static get style() { return { ios: myTagIosStyle }; }
const styleIdentifier = ts.factory.createIdentifier(style.styleIdentifier);
const propIdentifier = ts.factory.createPropertyAssignment(style.modeName, styleIdentifier);
styleModes.push(propIdentifier);
}
});

return ts.factory.createObjectLiteralExpression(styleModes, true);
};

const getSingleStyle = (cmp: d.ComponentCompilerMeta, style: d.StyleCompiler, commentOriginalSelector: boolean) => {
/**
* the order of these if statements must match with
* - {@link src/compiler/transformers/component-native/native-static-style.ts#addSingleStyleGetter}
* - {@link src/compiler/transformers/component-native/native-static-style.ts#addMultipleModeStyleGetter}
* - {@link src/compiler/transformers/add-static-style.ts#getSingleStyle}
*/
if (typeof style.styleStr === 'string') {
// inline the style string
// static get style() { return "string"; }
Expand Down
38 changes: 25 additions & 13 deletions src/compiler/transformers/component-native/native-static-style.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,26 +27,32 @@ const addMultipleModeStyleGetter = (
const styleModes: ts.ObjectLiteralElementLike[] = [];

styles.forEach((style) => {
/**
* the order of these if statements must match with
* - {@link src/compiler/transformers/component-native/native-static-style.ts#addSingleStyleGetter}
* - {@link src/compiler/transformers/add-static-style.ts#getSingleStyle}
* - {@link src/compiler/transformers/add-static-style.ts#getMultipleModeStyle}
*/
if (typeof style.styleStr === 'string') {
// inline the style string
// static get style() { return { "ios": "string" }; }
const styleLiteral = createStyleLiteral(cmp, style);
Copy link
Contributor

Choose a reason for hiding this comment

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

GH won't allow me to comment on a line higher than this one - can we add a comment before this conditional starts (above the if statement) that the order of the conditional must match add-static-style.ts#getSingleStyle?

Copy link
Contributor

Choose a reason for hiding this comment

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

on this point, I was thinking it might make sense to circle back after the bugfix is out and factor out this logic into a helper function since it looks essentially the same in these two spots

const propStr = ts.factory.createPropertyAssignment(style.modeName, styleLiteral);
styleModes.push(propStr);
} else if (typeof style.styleIdentifier === 'string') {
// direct import already written in the source code
// import myTagIosStyle from './import-path.css';
// static get style() { return { "ios": myTagIosStyle }; }
const styleIdentifier = ts.factory.createIdentifier(style.styleIdentifier);
const propIdentifier = ts.factory.createPropertyAssignment(style.modeName, styleIdentifier);
styleModes.push(propIdentifier);
} else if (Array.isArray(style.externalStyles) && style.externalStyles.length > 0) {
// import generated from @Component() styleUrls option
// import myTagIosStyle from './import-path.css';
// static get style() { return { "ios": myTagIosStyle }; }
const styleUrlIdentifier = createStyleIdentifier(cmp, style);
const propUrlIdentifier = ts.factory.createPropertyAssignment(style.modeName, styleUrlIdentifier);
styleModes.push(propUrlIdentifier);
} else if (typeof style.styleIdentifier === 'string') {
// direct import already written in the source code
// import myTagIosStyle from './import-path.css';
// static get style() { return { "ios": myTagIosStyle }; }
const styleIdentifier = ts.factory.createIdentifier(style.styleIdentifier);
const propIdentifier = ts.factory.createPropertyAssignment(style.modeName, styleIdentifier);
styleModes.push(propIdentifier);
}
});

Expand All @@ -60,23 +66,29 @@ const addSingleStyleGetter = (
cmp: d.ComponentCompilerMeta,
style: d.StyleCompiler,
) => {
/**
* the order of these if statements must match with
* - {@link src/compiler/transformers/component-native/native-static-style.ts#addMultipleModeStyleGetter}
* - {@link src/compiler/transformers/add-static-style.ts#getSingleStyle}
* - {@link src/compiler/transformers/add-static-style.ts#getMultipleModeStyle}
*/
if (typeof style.styleStr === 'string') {
// inline the style string
// static get style() { return "string"; }
Copy link
Contributor

Choose a reason for hiding this comment

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

GH won't allow me to comment on a line higher than this one - can we add a comment before this conditional starts (above the if statement) that the order of the conditional must match add-static-style.ts#getSingleStyle?

const styleLiteral = createStyleLiteral(cmp, style);
classMembers.push(createStaticGetter('style', styleLiteral));
} else if (typeof style.styleIdentifier === 'string') {
// direct import already written in the source code
// import myTagStyle from './import-path.css';
// static get style() { return myTagStyle; }
const styleIdentifier = ts.factory.createIdentifier(style.styleIdentifier);
classMembers.push(createStaticGetter('style', styleIdentifier));
} else if (Array.isArray(style.externalStyles) && style.externalStyles.length > 0) {
// import generated from @Component() styleUrls option
// import myTagStyle from './import-path.css';
// static get style() { return myTagStyle; }
const styleUrlIdentifier = createStyleIdentifier(cmp, style);
classMembers.push(createStaticGetter('style', styleUrlIdentifier));
} else if (typeof style.styleIdentifier === 'string') {
// direct import already written in the source code
// import myTagStyle from './import-path.css';
// static get style() { return myTagStyle; }
const styleIdentifier = ts.factory.createIdentifier(style.styleIdentifier);
classMembers.push(createStaticGetter('style', styleIdentifier));
}
};

Expand Down
43 changes: 43 additions & 0 deletions src/compiler/transformers/test/native-component.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,4 +166,47 @@ describe('nativeComponentTransform', () => {
);
});
});

describe('static style property', () => {
it.each([
[`styleUrl: 'cmp-a.css'`, `import CmpAStyle0 from './cmp-a.css?tag=cmp-a';`, `CmpAStyle0`],
[
`styleUrls: ['cmp-a.css', 'cmp-b.css', 'cmp-a.css']`,
`import CmpAStyle0 from './cmp-b.css?tag=cmp-a';
import CmpAStyle1 from './cmp-a.css?tag=cmp-a';`,
`CmpAStyle0 + CmpAStyle1`,
],
[
`styleUrls: { ios: 'cmp-a.ios.css', md: 'cmp-a.md.css' }`,
`import CmpAIosStyle0 from './cmp-a.ios.css?tag=cmp-a&mode=ios';
import CmpAMdStyle0 from './cmp-a.md.css?tag=cmp-a&mode=md';`,
`{ ios: CmpAIosStyle0, md: CmpAMdStyle0 }`,
],
])('adds a static style property when %s', async (styleConfig, expectedImport, expectedStyleReturn) => {
const code = `
@Component({
tag: 'cmp-a',
${styleConfig}
}
export class CmpA {}
`;
const transformer = nativeComponentTransform(compilerCtx, transformOpts);
const transpiledModule = transpileModule(code, null, compilerCtx, [], [transformer]);

expect(await formatCode(transpiledModule.outputText)).toContain(
await formatCode(`import { defineCustomElement as __stencil_defineCustomElement, HTMLElement } from '@stencil/core';
${expectedImport}
const CmpA = class extends HTMLElement {
constructor() {
super();
this.__registerHost();
}
static get style() {
return ${expectedStyleReturn};
}
};
`),
);
});
});
});