fix: handle multiple url() #237

layershifter · 2022-10-03T14:06:08Z

Fixes #238. See the bug for the issue description in detail.

The main change is that we use tokenize from stylis instead of regexps. This is more robust and allows to parse multiple url() expressions.

@griffel/babel-preset and @griffel/webpack-extraction-plugin were updated/adjusted to handle this scenario.

github-actions · 2022-10-03T14:07:23Z

📊 Bundle size report

_{🤖 This report was generated against 28641c5bc7616ffe3ec327b0de67625bbd9c727c}

layershifter · 2022-10-03T14:21:43Z

packages/babel-preset/src/assets/isAssetUrl.ts

@@ -0,0 +1,3 @@
+export function isAssetUrl(value: string): boolean {


I intentionally didn't cover this by tests directly as it will be changed/removed in #234.

layershifter · 2022-10-03T14:22:43Z

packages/babel-preset/src/assets/normalizeStyleRules.ts

-  // Quotes in URL are optional, so we can also normalize them
-  // https://www.w3.org/TR/CSS2/syndata.html#value-def-uri
-  const url = result.url.replace(/['|"](.+)['|"]/, '$1');
+  return tokenize(ruleValue).reduce((result, token, index, array) => {


Instead of a regex I am using tokenize() from stylis so we can parse all url() entries properly.

layershifter · 2022-10-03T14:28:04Z

packages/babel-preset/src/assets/replaceAssetsWithImports.ts

+    const quasis: t.TemplateElement[] = [];
+    const expressions: t.Identifier[] = [];
+
+    let acc = '';
+
+    for (let i = 0, l = tokens.length; i < l; i++) {
+      for (; i < l; i++) {
+        acc += tokens[i];
+
+        if (tokens[i] === 'url') {
+          const url = tokens[i + 1].slice(1, -1);
+
+          if (isAssetUrl(url)) {
+            quasis.push(t.templateElement({ raw: acc + '(' }, false));
+            expressions.push(getAssetIdentifier(url));
+
+            acc = ')';
+            i++;
+
+            break;
+          }
+        }
+      }
+    }
+
+    quasis.push(t.templateElement({ raw: acc }, true));


It's not the loop that makes me proud, but it works ¯_(ツ)_/¯ (opened for suggestions).

Based on tokens ('.foo', '{', 'url', '(image.png)', '}') we need to create:

N TemplateElements (t.templateElement({ raw: '.foo{url(' }, false), t.templateElement({ raw: ')}' }, true))

N-1 expressions (t.identifier('image.png'))

N & N-1 are a requirement as it's how Babel manages template literals...

I can't think of much better....

but I don't quite understand why you need the second nested loop, once you find the url token you break out of the inner loop and the outer one continues where the inner one left off anyway ??

but I don't quite understand why you need the second nested loop, once you find the url token you break out of the inner loop and the outer one continues where the inner one left off anyway ??

Good catch, it's a leftover from the previous attempt. Removed.

layershifter · 2022-10-03T14:28:24Z

packages/babel-preset/src/transformPlugin.test.ts

-    {
-      title: 'assets import handling',
-      fixture: path.resolve(fixturesDir, 'assets', 'code.ts'),
-      outputFixture: path.resolve(fixturesDir, 'assets', 'output.ts'),
-    },


Moved into a separate group.

layershifter · 2022-10-03T14:29:06Z

packages/webpack-extraction-plugin/src/babelPluginStripGriffelRuntime.ts

+                  const expressionPaths = literalPath.get('expressions');
+
+                  expressionPaths.map(expressionPath => {


The inner part is 100% percent the same: now we parse all expressions instead of only first.

ling1726 · 2022-10-11T11:24:49Z

packages/babel-preset/src/assets/replaceAssetsWithImports.ts

+  function buildTemplateLiteralFromValue(value: string): t.TemplateLiteral {
+    const tokens = tokenize(value);
+
+    const quasis: t.TemplateElement[] = [];


what are quasis ?

It's how Babel names string in template literals i.e. foo from foo${bar}:

https://babeljs.io/docs/en/babel-types#templateliteral

ling1726 · 2022-10-13T05:47:33Z

packages/babel-preset/src/assets/replaceAssetsWithImports.ts

+        acc += tokens[i];
+
+        if (tokens[i] === 'url') {
+          const url = tokens[i + 1].slice(1, -1);


why ignore the first and last characters of a url ?

I'm going by your example of tokens: ('.foo', '{', 'url', 'image.png', '}')

The first and last characters are parens. url(image.png) tokenizes to ["url", "(image.png)"]

ling1726

I have doubts about the ugly nested loop in the preset, but otherwise LGTM

layershifter mentioned this pull request Oct 3, 2022

fix: don't extract http:, https:, data: and hash only urls as assets #234

Merged

layershifter force-pushed the fix/handle-multiple-urls branch from 2d0b301 to 7fa5811 Compare October 3, 2022 14:20

layershifter commented Oct 3, 2022

View reviewed changes

layershifter marked this pull request as ready for review October 3, 2022 15:50

layershifter requested a review from a team as a code owner October 3, 2022 15:50

ling1726 reviewed Oct 11, 2022

View reviewed changes

ling1726 reviewed Oct 13, 2022

View reviewed changes

fix: handle multiple url()

a3f91c9

layershifter force-pushed the fix/handle-multiple-urls branch from 7fa5811 to a3f91c9 Compare October 13, 2022 07:10

ling1726 approved these changes Oct 13, 2022

View reviewed changes

remove nested loop

e3c16f2

layershifter merged commit 32d9b0b into microsoft:main Oct 13, 2022

layershifter deleted the fix/handle-multiple-urls branch October 13, 2022 07:50

layershifter mentioned this pull request Oct 13, 2022

chore: Update Griffel to latest version microsoft/fluentui#25212

Merged

		@@ -0,0 +1,3 @@
		export function isAssetUrl(value: string): boolean {

		const expressionPaths = literalPath.get('expressions');

		expressionPaths.map(expressionPath => {

fix: handle multiple url() #237

fix: handle multiple url() #237

Uh oh!

Conversation

layershifter commented Oct 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Oct 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📊 Bundle size report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

layershifter Oct 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

layershifter Oct 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ling1726 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

layershifter commented Oct 3, 2022 •

edited

Loading

github-actions bot commented Oct 3, 2022 •

edited

Loading

layershifter Oct 3, 2022 •

edited

Loading

layershifter Oct 3, 2022 •

edited

Loading