-
Notifications
You must be signed in to change notification settings - Fork 77
fix: handle multiple url() #237
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| { | ||
| "type": "patch", | ||
| "comment": "fix: handle multiple url()", | ||
| "packageName": "@griffel/babel-preset", | ||
| "email": "olfedias@microsoft.com", | ||
| "dependentChangeType": "patch" | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| { | ||
| "type": "patch", | ||
| "comment": "fix: handle multiple url()", | ||
| "packageName": "@griffel/webpack-extraction-plugin", | ||
| "email": "olfedias@microsoft.com", | ||
| "dependentChangeType": "patch" | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| import { makeStyles } from '@griffel/react'; | ||
|
|
||
| import blank from './blank.jpg'; | ||
| import empty from './empty.jpg'; | ||
|
|
||
| export const useStyles = makeStyles({ | ||
| root: { backgroundImage: `url(${blank}), url(${empty})` }, | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| import _asset2 from './empty.jpg'; | ||
| import _asset from './blank.jpg'; | ||
| import { __styles } from '@griffel/react'; | ||
| import blank from './blank.jpg'; | ||
| import empty from './empty.jpg'; | ||
| export const useStyles = __styles( | ||
| { | ||
| root: { | ||
| Bcmaq0h: 'fp00rh9', | ||
| }, | ||
| }, | ||
| { | ||
| d: [`.fp00rh9{background-image:url(${_asset}),url(${_asset2});}`], | ||
| }, | ||
| ); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| import { makeStyles } from '@griffel/react'; | ||
|
|
||
| export const useStyles = makeStyles({ | ||
| data: { backgroundImage: 'url(data:image/jpeg;base64,/9j/4AAQSkZJRgABAQEAYABgAAD/2Q==)' }, | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| import { __styles } from '@griffel/react'; | ||
| export const useStyles = __styles( | ||
| { | ||
| data: { | ||
| Bcmaq0h: 'fclgv40', | ||
| }, | ||
| }, | ||
| { | ||
| d: [`.fclgv40{background-image:url(data:image/jpeg;base64,/9j/4AAQSkZJRgABAQEAYABgAAD/2Q==);}`], | ||
| }, | ||
| ); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| export function isAssetUrl(value: string): boolean { | ||
| return !value.startsWith('data:'); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,7 @@ | ||
| import { GriffelStyle } from '@griffel/core'; | ||
| import { parseStringWithUrl } from './parseStringWithUrl'; | ||
| import { tokenize } from 'stylis'; | ||
|
|
||
| import { isAssetUrl } from './isAssetUrl'; | ||
|
|
||
| /** | ||
| * Linaria v3 emits relative paths for assets, we normalize these paths to be relative from the project root to be the | ||
|
|
@@ -22,16 +24,19 @@ export function normalizeStyleRule( | |
| return ruleValue; | ||
| } | ||
|
|
||
| const result = parseStringWithUrl(ruleValue); | ||
| // 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) => { | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of a regex I am using |
||
| if (token === 'url') { | ||
| // Quotes in URL are optional, so we can also normalize them | ||
| // https://www.w3.org/TR/CSS2/syndata.html#value-def-uri | ||
| const url = array[index + 1].replace(/['|"](.+)['|"]/, '$1').slice(1, -1); | ||
|
|
||
| if (url.startsWith('data:')) { | ||
| return `${result.prefix}${url}${result.suffix}`; | ||
| } | ||
| if (isAssetUrl(url)) { | ||
| array[index + 1] = `(${normalizeAssetPath(path, projectRoot, filename, url)})`; | ||
| } | ||
| } | ||
|
|
||
| return `${result.prefix}${normalizeAssetPath(path, projectRoot, filename, url)}${result.suffix}`; | ||
| return result + token; | ||
| }, ''); | ||
| } | ||
|
|
||
| export function normalizeStyleRules( | ||
|
|
||
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,8 +1,9 @@ | ||
| import { NodePath, traverse, types as t } from '@babel/core'; | ||
| import * as path from 'path'; | ||
| import { tokenize } from 'stylis'; | ||
|
|
||
| import { absolutePathToRelative } from './absolutePathToRelative'; | ||
| import { parseStringWithUrl } from './parseStringWithUrl'; | ||
| import { isAssetUrl } from './isAssetUrl'; | ||
|
|
||
| /** | ||
| * Replaces assets used in styles with imports and template literals. | ||
|
|
@@ -29,6 +30,39 @@ export function replaceAssetsWithImports( | |
| return assetIdentifiers.get(assetPath) as t.Identifier; | ||
| } | ||
|
|
||
| function buildTemplateLiteralFromValue(value: string): t.TemplateLiteral { | ||
| const tokens = tokenize(value); | ||
|
|
||
| const quasis: t.TemplateElement[] = []; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what are quasis ?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's how Babel names string in template literals i.e. |
||
| 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); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why ignore the first and last characters of a url ?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm going by your example of tokens:
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The first and last characters are parens. |
||
|
|
||
| if (isAssetUrl(url)) { | ||
| quasis.push(t.templateElement({ raw: acc + '(' }, false)); | ||
| expressions.push(getAssetIdentifier(url)); | ||
|
|
||
| acc = ')'; | ||
| i++; | ||
|
|
||
| break; | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| quasis.push(t.templateElement({ raw: acc }, true)); | ||
|
Comment on lines
+36
to
+57
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's not the loop that makes me proud, but it works ¯_(ツ)_/¯ (opened for suggestions). Based on tokens (
N & N-1 are a requirement as it's how Babel manages template literals...
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can't think of much better.... but I don't quite understand why you need the second nested loop, once you find the
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Good catch, it's a leftover from the previous attempt. Removed. |
||
|
|
||
| return t.templateLiteral(quasis, expressions); | ||
| } | ||
|
|
||
| traverse( | ||
| pathToUpdate.node, | ||
| { | ||
|
|
@@ -39,15 +73,7 @@ export function replaceAssetsWithImports( | |
| return; | ||
| } | ||
|
|
||
| const result = parseStringWithUrl(value); | ||
| const assetIdentifier = getAssetIdentifier(result.url); | ||
|
|
||
| const templateLiteral = t.templateLiteral( | ||
| [t.templateElement({ raw: result.prefix }, false), t.templateElement({ raw: result.suffix }, true)], | ||
| [assetIdentifier], | ||
| ); | ||
|
|
||
| literalPath.replaceWith(templateLiteral); | ||
| literalPath.replaceWith(buildTemplateLiteralFromValue(value)); | ||
| }, | ||
| }, | ||
| programPath.scope, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -35,11 +35,6 @@ pluginTester({ | |
| // Fixtures | ||
| // | ||
| // | ||
| { | ||
| title: 'assets import handling', | ||
| fixture: path.resolve(fixturesDir, 'assets', 'code.ts'), | ||
| outputFixture: path.resolve(fixturesDir, 'assets', 'output.ts'), | ||
| }, | ||
|
Comment on lines
-38
to
-42
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Moved into a separate group. |
||
| { | ||
| title: 'duplicated imports', | ||
| fixture: path.resolve(fixturesDir, 'duplicated-imports', 'code.ts'), | ||
|
|
@@ -59,6 +54,25 @@ pluginTester({ | |
| outputFixture: path.resolve(fixturesDir, 'non-existing-module-call', 'output.ts'), | ||
| }, | ||
|
|
||
| // Assets | ||
| // | ||
| // | ||
| { | ||
| title: 'assets: import handling', | ||
| fixture: path.resolve(fixturesDir, 'assets', 'code.ts'), | ||
| outputFixture: path.resolve(fixturesDir, 'assets', 'output.ts'), | ||
| }, | ||
| { | ||
| title: 'assets: multiple url()', | ||
| fixture: path.resolve(fixturesDir, 'assets-multiple-declarations', 'code.ts'), | ||
| outputFixture: path.resolve(fixturesDir, 'assets-multiple-declarations', 'output.ts'), | ||
| }, | ||
| { | ||
| title: 'assets: url() without imports', | ||
| fixture: path.resolve(fixturesDir, 'assets-urls', 'code.ts'), | ||
| outputFixture: path.resolve(fixturesDir, 'assets-urls', 'output.ts'), | ||
| }, | ||
|
|
||
| // Evaluation | ||
| // | ||
| // | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| import { __styles } from '@griffel/react'; | ||
| import _asset from './blank.jpg'; | ||
| import _asset2 from './empty.jpg'; | ||
|
|
||
| export const useStyles = __styles( | ||
| { | ||
| root: { | ||
| Bcmaq0h: 'fp00rh9', | ||
| }, | ||
| }, | ||
| { | ||
| d: [`.fp00rh9{background-image:url(${_asset}),url(${_asset2});}`], | ||
| }, | ||
| ); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| ["blank.jpg", "bundle.js", "empty.jpg", "griffel.css"] |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| .fp00rh9 { | ||
| background-image: url(blank.jpg), url(empty.jpg); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| import { __styles, __css } from '@griffel/react'; | ||
| export const useStyles = __css({ | ||
| root: { | ||
| Bcmaq0h: 'fp00rh9', | ||
| }, | ||
| }); | ||
|
|
||
| import 'griffel.css!=!../../../virtual-loader/index.js!../../../virtual-loader/griffel.css?style=.fp00rh9%7Bbackground-image%3Aurl(..%2F__fixtures__%2Fwebpack%2Fassets-multiple%2Fblank.jpg)%2Curl(..%2F__fixtures__%2Fwebpack%2Fassets-multiple%2Fempty.jpg)%3B%7D'; |

There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I intentionally didn't cover this by tests directly as it will be changed/removed in #234.