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(compiler): ensure not to import duplicate style identifier #5119

Merged
merged 3 commits into from
Dec 1, 2023
Merged

fix(compiler): ensure not to import duplicate style identifier #5119

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
bfde85f
fix(compiler): ensure not to import duplicate style identifier
christian-bromann Dec 1, 2023
6512334
prettier
christian-bromann Dec 1, 2023
65a362a
set default module type to ESM
christian-bromann Dec 1, 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
23 changes: 14 additions & 9 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 @@ -91,7 +91,8 @@ const getMultipleModeStyle = (
// import generated from @Component() styleUrls option
// import myTagIosStyle from './import-path.css';
// static get style() { return { ios: myTagIosStyle }; }
const styleUrlIdentifier = createStyleIdentifierFromUrl(style.externalStyles);
const externalStyles = Array.from(new Set(style.externalStyles.map((s) => s.absolutePath)));
const styleUrlIdentifier = createStyleIdentifierFromUrl(style.styleId, externalStyles);
const propUrlIdentifier = ts.factory.createPropertyAssignment(style.modeName, styleUrlIdentifier);
styleModes.push(propUrlIdentifier);
}
Expand All @@ -118,7 +119,8 @@ const getSingleStyle = (cmp: d.ComponentCompilerMeta, style: d.StyleCompiler, co
// import generated from @Component() styleUrls option
// import myTagStyle from './import-path.css';
// static get style() { return myTagStyle; }
return createStyleIdentifierFromUrl(style.externalStyles);
const externalStyles = Array.from(new Set(style.externalStyles.map((s) => s.absolutePath)));
return createStyleIdentifierFromUrl(style.styleId, externalStyles);
}

return null;
Expand All @@ -141,6 +143,7 @@ const createStyleLiteral = (cmp: d.ComponentCompilerMeta, style: d.StyleCompiler
* ```ts
* @Component({
* styleUrls: ['my-component.css', 'my-component.ios.css']
* tag: 'cmp',
* })
* export class MyComponent {
* // ...
Expand All @@ -150,30 +153,32 @@ const createStyleLiteral = (cmp: d.ComponentCompilerMeta, style: d.StyleCompiler
* it would generate the following expression:
*
* ```ts
* import _myComponentCssStyle from './my-component.css';
* import _myComponentIosCssStyle from './my-component.ios.css';
* import CMP_my_component_css from './my-component.css';
* import CMP_my_component_ios_css from './my-component.ios.css';
* export class MyComponent {
* // ...
* }
* MyComponent.style = _myComponentCssStyle + _myComponentIosCssStyle;
* MyComponent.style = CMP_my_component_css + CMP_my_component_ios_css;
* ```
*
* Note: style imports are made in [`createEsmStyleImport`](src/compiler/transformers/style-imports.ts).
*
* @param styleId a unique identifier for the component style
* @param externalStyles a list of external styles to be applied the component
* @returns an assignment expression to be applied to the `style` property of a component class (e.g. `_myComponentCssStyle + _myComponentIosCssStyle` based on the example)
*/
export const createStyleIdentifierFromUrl = (
externalStyles: d.ExternalStyleCompiler[],
styleId: string,
externalStyles: string[],
): ts.Identifier | ts.BinaryExpression => {
if (externalStyles.length === 1) {
return ts.factory.createIdentifier(getIdentifierFromResourceUrl(externalStyles[0].absolutePath));
return ts.factory.createIdentifier(getIdentifierFromResourceUrl(styleId + externalStyles[0]));
}

const firstExternalStyle = externalStyles[0];
return ts.factory.createBinaryExpression(
createStyleIdentifierFromUrl([firstExternalStyle]),
createStyleIdentifierFromUrl(styleId, [firstExternalStyle]),
ts.SyntaxKind.PlusToken,
createStyleIdentifierFromUrl(externalStyles.slice(1)),
createStyleIdentifierFromUrl(styleId, externalStyles.slice(1)),
);
};
6 changes: 4 additions & 2 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 @@ -44,7 +44,8 @@ const addMultipleModeStyleGetter = (
// import generated from @Component() styleUrls option
// import myTagIosStyle from './import-path.css';
// static get style() { return { "ios": myTagIosStyle }; }
const styleUrlIdentifier = createStyleIdentifierFromUrl(style.externalStyles);
const externalStyles = Array.from(new Set(style.externalStyles.map((s) => s.absolutePath)));
const styleUrlIdentifier = createStyleIdentifierFromUrl(style.styleId, externalStyles);
const propUrlIdentifier = ts.factory.createPropertyAssignment(style.modeName, styleUrlIdentifier);
styleModes.push(propUrlIdentifier);
}
Expand Down Expand Up @@ -75,7 +76,8 @@ const addSingleStyleGetter = (
// import generated from @Component() styleUrls option
// import myTagStyle from './import-path.css';
// static get style() { return myTagStyle; }
const styleUrlIdentifier = createStyleIdentifierFromUrl(style.externalStyles);
const externalStyles = Array.from(new Set(style.externalStyles.map((s) => s.absolutePath)));
const styleUrlIdentifier = createStyleIdentifierFromUrl(style.styleId, externalStyles);
classMembers.push(createStaticGetter('style', styleUrlIdentifier));
}
};
Expand Down
152 changes: 93 additions & 59 deletions src/compiler/transformers/style-imports.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ const updateEsmStyleImports = (
statements = updateEsmStyleImportPath(transformOpts, tsSourceFile, statements, cmp, style);
} else if (style.externalStyles.length > 0) {
// add style imports built from @Component() styleUrl option
styleImports.push(...createEsmStyleImport(transformOpts, tsSourceFile, cmp, style));
styleImports.push(...createStyleImport(transformOpts, tsSourceFile, cmp, style, transformOpts.module as 'esm'));
}
});
});
Expand Down Expand Up @@ -120,31 +120,108 @@ const updateEsmStyleImportPath = (
return statements;
};

const createEsmStyleImport = (
/**
* Add import or require statement for each style
* e.g. `import CMP__import_path_css from './import-path.css';`
*
* @param transformOpts transform options configured for the current output target transpilation
* @param tsSourceFile the TypeScript source file that is being updated
* @param cmp component meta data
* @param style style meta data
* @param moduleType module type (either 'esm' or 'cjs')
* @returns an set or import or require statements to add to the source file
*/
const createStyleImport = <ModuleType extends 'esm' | 'cjs'>(
transformOpts: d.TransformOptions,
tsSourceFile: ts.SourceFile,
cmp: d.ComponentCompilerMeta,
style: d.StyleCompiler,
/**
* default to 'esm' if not provided, behavior defined in `updateStyleImports`
*/
moduleType: ModuleType = 'esm' as ModuleType,
) => {
const imports: ts.ImportDeclaration[] = [];
type ImportDeclarationOrVariableStatementType = ModuleType extends 'esm'
? ts.ImportDeclaration
: ts.VariableStatement;
const imports: ImportDeclarationOrVariableStatementType[] = [];
const importedStyleIdentifiers: string[] = [];

for (const externalStyle of style.externalStyles) {
/**
* Add import statement for each style
* e.g. `const _ImportPathStyle = require('./import-path.css');`
* Concat styleId and absolutePath to get a unique identifier for each style.
*
* For example:
* ```ts
* @Component({
* styleUrls: {
* md: './foo/bar.css',
* ios: './bar/foo.css'
* },
* tag: 'cmp-a'
* })
* ```
*
* Attention: if you make changes to the import identifier (e.g. `_ImportPathStyle`),
* you also need to update the identifier in [`createStyleIdentifierFromUrl`](`src/compiler/transformers/add-static-style.ts`).
* it would create the following identifiers:
* ```ts
* import CMP_A_md__foo_bar_css from './foo/bar.css';
* import CMP_A_ios__bar_foo_css from './bar/foo.css';
* ```
*
* Attention: if you make changes to how this identifier is created you also need
* to update this in [`createStyleIdentifierFromUrl`](`src/compiler/transformers/add-static-style.ts`).
*/
const styleIdentifier = getIdentifierFromResourceUrl(style.styleId + externalStyle.absolutePath);

/**
* avoid to have duplicate style imports, e.g. if we have the following component:
* ```ts
* @Component({
* styleUrls: ['./foo/bar.css', './foo/bar.css'],
* tag: 'cmp-a'
* })
* ```
*/
const importIdentifier = ts.factory.createIdentifier(getIdentifierFromResourceUrl(externalStyle.absolutePath));
if (importedStyleIdentifiers.includes(styleIdentifier)) {
continue;
}

importedStyleIdentifiers.push(styleIdentifier);
const importIdentifier = ts.factory.createIdentifier(styleIdentifier);
const importPath = getStyleImportPath(transformOpts, tsSourceFile, cmp, style, externalStyle.absolutePath);

imports.push(
ts.factory.createImportDeclaration(
undefined,
ts.factory.createImportClause(false, importIdentifier, undefined),
ts.factory.createStringLiteral(importPath),
),
);
if (moduleType === 'esm') {
imports.push(
ts.factory.createImportDeclaration(
undefined,
ts.factory.createImportClause(false, importIdentifier, undefined),
ts.factory.createStringLiteral(importPath),
) as ImportDeclarationOrVariableStatementType,
);
} else if (moduleType === 'cjs') {
imports.push(
ts.factory.createVariableStatement(
undefined,
ts.factory.createVariableDeclarationList(
[
ts.factory.createVariableDeclaration(
importIdentifier,
undefined,
undefined,
ts.factory.createCallExpression(
ts.factory.createIdentifier('require'),
[],
[ts.factory.createStringLiteral(importPath)],
),
),
],
ts.NodeFlags.Const,
),
) as ImportDeclarationOrVariableStatementType,
);
} else {
throw new Error(`Invalid module type: ${moduleType}`);
}
}

return imports;
Expand All @@ -169,7 +246,7 @@ const updateCjsStyleRequires = (
cmp.styles.forEach((style) => {
if (style.externalStyles.length > 0) {
// add style imports built from @Component() styleUrl option
styleRequires.push(...createCjsStyleRequire(transformOpts, tsSourceFile, cmp, style));
styleRequires.push(...createStyleImport(transformOpts, tsSourceFile, cmp, style, transformOpts.module));
}
});
});
Expand All @@ -181,49 +258,6 @@ const updateCjsStyleRequires = (
return tsSourceFile;
};

const createCjsStyleRequire = (
transformOpts: d.TransformOptions,
tsSourceFile: ts.SourceFile,
cmp: d.ComponentCompilerMeta,
style: d.StyleCompiler,
) => {
const imports: ts.VariableStatement[] = [];
for (const externalStyle of style.externalStyles) {
/**
* Add import statement for each style
* e.g. `import _ImportPathStyle from './import-path.css';`
*
* Attention: if you make changes to the import identifier (e.g. `_ImportPathStyle`),
* you also need to update the identifier in [`createStyleIdentifierFromUrl`](`src/compiler/transformers/add-static-style.ts`).
*/
const importIdentifier = ts.factory.createIdentifier(getIdentifierFromResourceUrl(externalStyle.absolutePath));
const importPath = getStyleImportPath(transformOpts, tsSourceFile, cmp, style, externalStyle.absolutePath);

imports.push(
ts.factory.createVariableStatement(
undefined,
ts.factory.createVariableDeclarationList(
[
ts.factory.createVariableDeclaration(
importIdentifier,
undefined,
undefined,
ts.factory.createCallExpression(
ts.factory.createIdentifier('require'),
[],
[ts.factory.createStringLiteral(importPath)],
),
),
],
ts.NodeFlags.Const,
),
),
);
}

return imports;
};

const getStyleImportPath = (
transformOpts: d.TransformOptions,
tsSourceFile: ts.SourceFile,
Expand Down
60 changes: 48 additions & 12 deletions src/compiler/transformers/test/lazy-component.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ describe('lazy-component', () => {
);
});

it('allows to define multiple styleUrls', async () => {
it('allows to define multiple styleUrls in ESM', async () => {
const compilerCtx = mockCompilerCtx();
const transformOpts: d.TransformOptions = {
coreImportPath: '@stencil/core',
Expand All @@ -118,6 +118,7 @@ describe('lazy-component', () => {
currentDirectory: '/',
proxy: null,
style: 'static',
module: 'esm',
styleImportData: null,
};
const code = `
Expand All @@ -131,18 +132,18 @@ describe('lazy-component', () => {
const t = transpileModule(code, null, compilerCtx, [], [transformer]);
expect(await formatCode(t.outputText)).toBe(
await c`import { registerInstance as __stencil_registerInstance } from "@stencil/core";
import __foo_bar_css from './foo/bar.css';
import __bar_foo_css from './bar/foo.css';
import CMP_A__foo_bar_css from './foo/bar.css';
import CMP_A__bar_foo_css from './bar/foo.css';
export const CmpA = class {
constructor(hostRef) {
__stencil_registerInstance(this, hostRef);
}
}
CmpA.style = __foo_bar_css + __bar_foo_css ;`,
CmpA.style = CMP_A__foo_bar_css + CMP_A__bar_foo_css ;`,
);
});

it('allows to define multiple styleUrls in CJS', async () => {
it('allows to define multiple different styleUrls in CJS', async () => {
const compilerCtx = mockCompilerCtx();
const transformOpts: d.TransformOptions = {
coreImportPath: '@stencil/core',
Expand All @@ -164,15 +165,48 @@ describe('lazy-component', () => {
const transformer = lazyComponentTransform(compilerCtx, transformOpts);
const t = transpileModule(code, null, compilerCtx, [], [transformer]);
expect(await formatCode(t.outputText)).toBe(
await c`const __foo_bar_css = require('./foo/bar.css');
const __bar_foo_css = require('./bar/foo.css');
await c`const CMP_A__foo_bar_css = require('./foo/bar.css');
const CMP_A__bar_foo_css = require('./bar/foo.css');
const { registerInstance: __stencil_registerInstance } = require('@stencil/core');
export class CmpA {
constructor(hostRef) {
__stencil_registerInstance(this, hostRef);
}
};
CmpA.style = __foo_bar_css + __bar_foo_css ;`,
CmpA.style = CMP_A__foo_bar_css + CMP_A__bar_foo_css ;`,
);
});

it('can not have duplicate style urls', async () => {
const compilerCtx = mockCompilerCtx();
const transformOpts: d.TransformOptions = {
coreImportPath: '@stencil/core',
componentExport: 'lazy',
componentMetadata: null,
currentDirectory: '/',
proxy: null,
module: 'cjs',
style: 'static',
styleImportData: null,
};
const code = `
@Component({
styleUrls: ['./foo/bar.css', './foo/bar.css'],
tag: 'cmp-a'
})
export class CmpA {}
`;
const transformer = lazyComponentTransform(compilerCtx, transformOpts);
const t = transpileModule(code, null, compilerCtx, [], [transformer]);
expect(await formatCode(t.outputText)).toBe(
await c`const CMP_A__foo_bar_css = require('./foo/bar.css');
export class CmpA {
constructor(hostRef) {
__stencil_registerInstance(this, hostRef);
}
};
const { registerInstance: __stencil_registerInstance } = require('@stencil/core');
CmpA.style = CMP_A__foo_bar_css;`,
);
});

Expand All @@ -191,7 +225,8 @@ describe('lazy-component', () => {
@Component({
styleUrls: {
foo: './foo/bar.css',
bar: './bar/foo.css'
bar: './bar/foo.css',
loo: './bar/foo.css'
},
tag: 'cmp-a'
})
Expand All @@ -201,14 +236,15 @@ describe('lazy-component', () => {
const t = transpileModule(code, null, compilerCtx, [], [transformer]);
expect(await formatCode(t.outputText)).toBe(
await c`import { registerInstance as __stencil_registerInstance } from "@stencil/core";
import __bar_foo_css from './bar/foo.css';
import __foo_bar_css from './foo/bar.css';
import CMP_A_bar__bar_foo_css from './bar/foo.css';
import CMP_A_foo__foo_bar_css from './foo/bar.css';
import CMP_A_loo__bar_foo_css from './bar/foo.css';
export const CmpA = class {
constructor(hostRef) {
__stencil_registerInstance(this, hostRef);
}
}
CmpA.style = { bar: __bar_foo_css , foo: __foo_bar_css }`,
CmpA.style = { bar: CMP_A_bar__bar_foo_css , foo: CMP_A_foo__foo_bar_css, loo: CMP_A_loo__bar_foo_css }`,
);
});
});