fix(compiler): support multiple styleUrls in components #5090

christian-bromann · 2023-11-20T23:26:25Z

What is the current behavior?

Currently if multiple style urls are provided, as described in the docs:

@Component({
  tag: 'my-component',
  styleUrls: ['reset.scss', 'roboto.scss', 'css-variables.scss', 'core.scss'] 
})
export class MyComponent {
  // ...
}

Only the first (reset.css) style would get applied to the component.

GitHub Issue Number: #5016

What is the new behavior?

Instead of just compiling the first external style, I make it loop through all external styles and join the style strings together. Here is an example:

const barCss = ":host{display:block}";
const fooCss = ":host{display:none}";

const MyComponent = class {
    // ...
};
MyComponent.style = barCss + fooCss;

export { MyComponent as my_component };

Does this introduce a breaking change?

Yes
No

Testing

Working on adding unit tests and integration tests for it.

Other information

github-actions · 2023-11-20T23:27:48Z

`--strictNullChecks` error report

Typechecking with --strictNullChecks resulted in 1364 errors on this branch.

Unfortunately, it looks like that's an increase of 1 over main 😞.

Violations Not on `main` (may be more than the count above)

Path	Location	Error
src/compiler/transformers/stencil-import-path.ts	(68, 5)	TS2322
src/compiler/transformers/stencil-import-path.ts	(69, 5)	TS2322
src/compiler/transformers/stencil-import-path.ts	(70, 5)	TS2322
src/compiler/transformers/transform-utils.ts	(1112, 5)	TS2532

reports and statistics

Our most error-prone files

Path	Error Count
src/dev-server/index.ts	37
src/mock-doc/serialize-node.ts	36
src/dev-server/server-process.ts	32
src/compiler/build/build-stats.ts	23
src/compiler/style/test/optimize-css.spec.ts	23
src/testing/puppeteer/puppeteer-element.ts	23
src/compiler/output-targets/dist-lazy/generate-lazy-module.ts	22
src/compiler/prerender/prerender-main.ts	22
src/runtime/client-hydrate.ts	20
src/screenshot/connector-base.ts	19
src/runtime/vdom/vdom-render.ts	18
src/compiler/config/test/validate-paths.spec.ts	16
src/dev-server/request-handler.ts	15
src/compiler/prerender/prerender-optimize.ts	14
src/compiler/sys/stencil-sys.ts	14
src/compiler/transpile/transpile-module.ts	14
src/runtime/vdom/vdom-annotations.ts	14
src/sys/node/node-sys.ts	14
src/compiler/build/build-finish.ts	13
src/compiler/prerender/prerender-queue.ts	13

Our most common errors

Typescript Error Code	Count
TS2345	403
TS2322	385
TS18048	304
TS18047	101
TS2722	38
TS2532	35
TS2531	23
TS2454	14
TS2352	13
TS2769	10
TS2790	10
TS2538	8
TS2416	6
TS2344	5
TS2493	3
TS2488	2
TS18046	2
TS2684	1
TS2430	1

Unused exports report

There are 14 unused exports on this PR. That's the same number of errors on main, so at least we're not creating new ones!

Unused exports

File	Line	Identifier
src/runtime/bootstrap-lazy.ts	21	setNonce
src/screenshot/screenshot-fs.ts	18	readScreenshotData
src/testing/testing-utils.ts	198	withSilentWarn
src/utils/index.ts	145	CUSTOM
src/utils/index.ts	269	normalize
src/utils/index.ts	7	escapeRegExpSpecialCharacters
src/compiler/app-core/app-data.ts	25	BUILD
src/compiler/app-core/app-data.ts	115	Env
src/compiler/app-core/app-data.ts	117	NAMESPACE
src/compiler/fs-watch/fs-watch-rebuild.ts	123	updateCacheFromRebuild
src/compiler/types/validate-primary-package-output-target.ts	61	satisfies
src/compiler/types/validate-primary-package-output-target.ts	61	Record
src/testing/puppeteer/puppeteer-declarations.ts	485	WaitForEventOptions
src/compiler/sys/fetch/write-fetch-success.ts	7	writeFetchSuccessSync

christian-bromann · 2023-11-20T23:53:18Z

src/compiler/transformers/component-native/native-static-style.ts

@@ -88,17 +89,3 @@ const createStyleLiteral = (cmp: d.ComponentCompilerMeta, style: d.StyleCompiler

  return ts.factory.createStringLiteral(style.styleStr);
 };
-
-const createStyleIdentifierFromUrl = (cmp: d.ComponentCompilerMeta, style: d.StyleCompiler) => {


It seems to be the same function as in src/compiler/transformers/add-static-style.ts. Please let me know if there is any reason why the code was duplicated. Other functions in this file have also the same implementation with minor nuances. My concern here is that maybe some fixes were applied in one file but not the other.

I think this is a safe move to make 👍 Good find!

src/runtime/test/fixtures/cmp-a.tsx

This reverts commit a1ca049.

christian-bromann · 2023-11-27T18:37:28Z

src/compiler/transformers/stencil-import-path.ts

@@ -28,7 +29,7 @@ export const serializeImportPath = (data: SerializeImportData, styleImportData:
      p = './' + p;
    }

-    if (styleImportData === 'queryparams' || styleImportData === undefined) {
+    if (moduleSystem !== 'cjs' && (styleImportData === 'queryparams' || styleImportData === undefined)) {


I saw the following failures in unit tests:

Cannot find module './my-component.css?tag=my-component&encapsulation=shadow' from 'src/components/my-component/my-component.tsx'

as Jest tried to parse and run the component that would require the css files with queryparams don't have any function CJS land and confuse Jests resolver. This tweak makes sure we don't attach them when we are explicitly transforming to CJS.

is it possible this would cause problems if a user is trying to transpile to commonjs?

It depends I guess. Using query params in require calls is actually not supported and would fail, e.g.

CJS:

const bar = require('./b?foo=bar') // throws `Error: Cannot find module './b?foo=bar'`

ESM:

import b from './b.js?foo=bar' // works 👍

Now, I can imagine that there are use cases where folks compile it into CJS which then gets picked up by another bundler that compiles it back in ESM. If we assume that the compiler respects the require params and ports them along than it could introduce issues.

ok cool, I'm not worried then!

christian-bromann · 2023-11-27T18:39:56Z

This is ready for review now 🙌

alicewriteswrongs

tried it out locally and it looks great! Just had some comments and questions when reading through

alicewriteswrongs · 2023-11-29T16:48:49Z

src/compiler/transformers/add-static-style.ts

+    return ts.factory.createIdentifier(getIdentifierFromResourceUrl(externalStyles[0].absolutePath));
  }


is it right to say that we could just use the pascal-case version of the tag name for the identifier before because we were always just using the first entry, so we didn't have to worry about name collisions or anything? I guess know we know that the filenames specified in styleUrls have to be unique so if we have a one-to-one map from the absolutePath to the style identifier then we'll never have a collision

That is correct, before we used the component name as identifier which we can't anymore when importing multiple style sheets. getIdentifierFromResourceUrl is intended to return a unique identifier based on the absolute path. Now this has its own challenges, re special characters etc. An alternative would be to create a md5 sha based on the absolute path.

Note that the code that handles the imports and the one handling attaching them to the Component class are located in different functions so the only common identifier information is the absolute path.

alicewriteswrongs · 2023-11-29T16:49:08Z

src/compiler/transformers/add-static-style.ts

+  return ts.factory.createBinaryExpression(
+    createStyleIdentifierFromUrl([firstExternalStyle]),
+    ts.SyntaxKind.PlusToken,
+    createStyleIdentifierFromUrl(externalStyles.slice(1)),


so if we have, say, 3 styleUrls this will just recursively build up a string by adding another binary expression? nice!

Correct 😉

src/compiler/transformers/add-static-style.ts

alicewriteswrongs · 2023-11-29T16:52:31Z

src/compiler/transformers/add-static-style.ts

+ * Note: style imports are made in [`createEsmStyleImport`](src/compiler/transformers/style-imports.ts).
+ *


oh I guess this explains that haha

alicewriteswrongs · 2023-11-29T16:58:02Z

src/compiler/transformers/stencil-import-path.ts

@@ -28,7 +29,7 @@ export const serializeImportPath = (data: SerializeImportData, styleImportData:
      p = './' + p;
    }

-    if (styleImportData === 'queryparams' || styleImportData === undefined) {
+    if (moduleSystem !== 'cjs' && (styleImportData === 'queryparams' || styleImportData === undefined)) {


is it possible this would cause problems if a user is trying to transpile to commonjs?

alicewriteswrongs · 2023-11-29T17:00:00Z

src/compiler/transformers/style-imports.ts

+    // e.g. `import _ImportPathStyle from './import-path.css';`
+    const importName = getIdentifierFromResourceUrl(externalStyle.absolutePath);
+    const importIdentifier = ts.factory.createIdentifier(importName);
+    const importPath = getStyleImportPath(transformOpts, tsSourceFile, cmp, style, externalStyle.absolutePath);


maybe we should add a note here that we have implicit coupling between the creation of the import identifier here and the style getters we create elsewhere? I'd just want to make sure that anyone who might want to change this code in the future knows that the content of importIdentifier here is quite important, and if it's changed we'd need to make changes in a bunch of other spots too

I will add additional comments to address this.

alicewriteswrongs · 2023-11-29T17:10:15Z

src/compiler/transformers/transform-utils.ts

+const SPECIAL_CHARS = /[\s~`!@#$%\^&*+=\-\[\]\\';,/{}|\\":<>\?()\._]/g;
+/**
+ * transform any path to a valid identifier, e.g.
+ *   - `/foo/bar/loo.css` -> `_fooBarLooCssStyle`
+ *   - `C:\\foo\bar\loo.css` -> `_cFooBarLooCssStyle`
+ *
+ * @param absolutePath  windows or linux based path
+ * @returns a valid identifier to be used as variable name
+ */
+export const getIdentifierFromResourceUrl = (absolutePath: string): string => {
+  return `_${dashToPascalCase(
+    absolutePath.split('?').shift().toLocaleLowerCase().replace(SPECIAL_CHARS, '-').replace(/--/, '-'),
+  )}Style`;
+};


question - do you think we need to worry about escaping special characters here, rather than just removing them? I was able to generate a collision between two different valid POSIX paths like so (in the spec file):

expect( getIdentifierFromResourceUrl("/foo--bar.css") ).not.toEqual( getIdentifierFromResourceUrl("/foo-bar.css") )

they both get transformed to _FooBarCssStyle.css

I can trigger this condition from a component if I have a styleUrls array like so:

I get a rollup error because of variable redeclaration (or import identifier re-declaration I guess)

Good catch! I will make a tweak to the function to address this.

tanner-reits

LGTM w/ changes from Alice's comments! Appreciate all the code commenting you did as well!

tanner-reits · 2023-11-29T23:15:58Z

src/compiler/transformers/component-native/native-static-style.ts

@@ -88,17 +89,3 @@ const createStyleLiteral = (cmp: d.ComponentCompilerMeta, style: d.StyleCompiler

  return ts.factory.createStringLiteral(style.styleStr);
 };
-
-const createStyleIdentifierFromUrl = (cmp: d.ComponentCompilerMeta, style: d.StyleCompiler) => {


I think this is a safe move to make 👍 Good find!

alicewriteswrongs

looks good to me!

We recently landed changes in main (not released) related to support multiple style urls. These changes however have caused failures when building upstream project, e.g. in Ionic framework (see https://github.com/ionic-team/ionic-framework/actions/runs/7064837482/job/19233697164). This patch reverts these changes to unblock upcoming releases. Revert "fix(compiler): ensure not to import duplicate style identifier (#5119)" This reverts commit 7591dce. Revert "fix(compiler): support multiple styleUrls in components (#5090)" This reverts commit 54e52da.

fix(compiler): support multiple styleUrls in components

21386b7

apply change also to native component transformer

55dd957

christian-bromann commented Nov 20, 2023

View reviewed changes

add tests

b2d478d

christian-bromann marked this pull request as ready for review November 21, 2023 00:23

christian-bromann requested a review from a team as a code owner November 21, 2023 00:23

christian-bromann requested review from rwaskiewicz and tanner-reits November 21, 2023 00:23

christian-bromann added 7 commits November 20, 2023 16:34

prettier

4eb4e88

add another unit test

f1a5125

remove special characters properly

2c19c45

prettier fix

15f6698

split parameter off var name

1d53123

fix build issue

1eb9098

update test

a1ca049

christian-bromann commented Nov 22, 2023

View reviewed changes

src/runtime/test/fixtures/cmp-a.tsx Outdated Show resolved Hide resolved

christian-bromann added 3 commits November 21, 2023 19:06

Revert "update test"

3b9d376

This reverts commit a1ca049.

don't attach queryparams when compiling to CJS

e5c880a

prettier fix

85eb6c5

christian-bromann commented Nov 27, 2023

View reviewed changes

alicewriteswrongs suggested changes Nov 29, 2023

View reviewed changes

christian-bromann added 4 commits November 29, 2023 10:34

add additional comments to address coupling of identifier generation

c5a7ad9

make getIdentifierFromResourceUrl more robust

e46776c

remove import

223d631

prettier

13ac1f9

tanner-reits approved these changes Nov 29, 2023

View reviewed changes

alicewriteswrongs approved these changes Nov 30, 2023

View reviewed changes

christian-bromann added this pull request to the merge queue Nov 30, 2023

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 30, 2023

christian-bromann added this pull request to the merge queue Nov 30, 2023

Merged via the queue into main with commit 54e52da Nov 30, 2023
120 checks passed

christian-bromann deleted the cb/fix-5016 branch November 30, 2023 19:42

christian-bromann mentioned this pull request Dec 4, 2023

fix(compiler): revert changes to style import transformer #5125

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(compiler): support multiple styleUrls in components #5090

fix(compiler): support multiple styleUrls in components #5090

christian-bromann commented Nov 20, 2023

github-actions bot commented Nov 20, 2023 •

edited

Loading

christian-bromann Nov 20, 2023

tanner-reits Nov 29, 2023

christian-bromann Nov 27, 2023 •

edited

Loading

alicewriteswrongs Nov 29, 2023

christian-bromann Nov 29, 2023

alicewriteswrongs Nov 29, 2023

christian-bromann commented Nov 27, 2023

alicewriteswrongs left a comment

alicewriteswrongs Nov 29, 2023

christian-bromann Nov 29, 2023

alicewriteswrongs Nov 29, 2023

christian-bromann Nov 29, 2023

alicewriteswrongs Nov 29, 2023

alicewriteswrongs Nov 29, 2023

alicewriteswrongs Nov 29, 2023

christian-bromann Nov 29, 2023

alicewriteswrongs Nov 29, 2023

alicewriteswrongs Nov 29, 2023

christian-bromann Nov 29, 2023

tanner-reits left a comment

tanner-reits Nov 29, 2023

alicewriteswrongs left a comment

		return ts.factory.createIdentifier(getIdentifierFromResourceUrl(externalStyles[0].absolutePath));
		}

		* Note: style imports are made in [`createEsmStyleImport`](src/compiler/transformers/style-imports.ts).
		*

fix(compiler): support multiple styleUrls in components #5090

fix(compiler): support multiple styleUrls in components #5090

Conversation

christian-bromann commented Nov 20, 2023

What is the current behavior?

What is the new behavior?

Does this introduce a breaking change?

Testing

Other information

github-actions bot commented Nov 20, 2023 • edited Loading

--strictNullChecks error report

reports and statistics

Unused exports report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christian-bromann Nov 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christian-bromann commented Nov 27, 2023

alicewriteswrongs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tanner-reits left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alicewriteswrongs left a comment

Choose a reason for hiding this comment

github-actions bot commented Nov 20, 2023 •

edited

Loading

`--strictNullChecks` error report

christian-bromann Nov 27, 2023 •

edited

Loading