Skip to content

Commit

Permalink
fix: improve performance of styling extractor
Browse files Browse the repository at this point in the history
  • Loading branch information
guillaumerochelle committed Jan 31, 2025
1 parent 2dfc7b8 commit c24ca1e
Show file tree
Hide file tree
Showing 7 changed files with 1,095 additions and 301 deletions.
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -260,6 +260,7 @@
"sass": "~1.81.0",
"sass-loader": "^14.0.0",
"semver": "^7.5.2",
"source-map-js": "^1.2.1",
"stylelint": "^16.0.2",
"stylelint-scss": "^6.0.0",
"ts-jest": "~29.2.0",
Expand Down
4 changes: 2 additions & 2 deletions packages/@o3r/design/schematics/extract-token/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -54,12 +54,12 @@ function extractTokenFn(options: ExtractTokenSchematicsSchema): Rule {
));
const sassParser = new CssVariableExtractor();

tree.visit((file) => {
tree.visit(async (file) => {
if (!filterFunctions.some((filterFunction) => filterFunction(file))) {
return;
}
const content = tree.readText(file);
const variables = sassParser.extractFileContent(resolve(tree.root.path, file), content);
const variables = await sassParser.extractFileContent(resolve(tree.root.path, file), content);

if (variables.length > 0 && options.includeTags) {
const newContent = updateFileContent(content);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import {
} from 'node:path';
import {
Logger,
} from 'sass';
} from 'sass-embedded';
import {
CssVariableExtractor,
} from './css-variable.extractor';
Expand All @@ -15,20 +15,20 @@ const logger = Logger.silent;

describe('CSS Variable extractor', () => {
describe('Sass file content parsing', () => {
test('should correctly extract variable details', () => {
test('should correctly extract variable details', async () => {
const mock = `@use '${testedFile}' as o3r;
:root {
@include o3r.var('my-var', #fff, (category: 'test category'));
}
`;

const parser = new CssVariableExtractor({ logger });
const variables = parser.extractFileContent(file, mock, { url });
const variables = await parser.extractFileContent(file, mock, { url });
expect(variables).toHaveLength(1);
expect(variables[0]).toEqual(expect.objectContaining({ name: 'my-var', defaultValue: 'rgba(255, 255, 255, 1)', category: 'test category' }));
});

test('should override variable details', () => {
test('should override variable details', async () => {
const mock = `@use '${testedFile}' as o3r;
:root {
@include o3r.var('my-var', #fff, (category: 'test category'));
Expand All @@ -37,7 +37,7 @@ describe('CSS Variable extractor', () => {
`;

const parser = new CssVariableExtractor({ logger });
const variables = parser.extractFileContent(file, mock, { url });
const variables = await parser.extractFileContent(file, mock, { url });
expect(variables).toHaveLength(1);
expect(variables[0]).toEqual(expect.objectContaining({ name: 'my-var', defaultValue: 'rgba(255, 255, 255, 1)', category: 'new category' }));
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,16 +10,22 @@ import {
O3rCliError,
} from '@o3r/schematics';
import {
compileString,
SassBoolean,
AsyncCompiler,
CalculationInterpolation,
CalculationOperation,
CalculationValue,
initAsyncCompiler,
SassCalculation,
SassColor,
sassFalse,
SassList,
SassMap,
SassNumber,
SassString,
sassTrue,
StringOptions,
Value,
} from 'sass';
} from 'sass-embedded';
import type {
StyleExtractorBuilderSchema,
} from '../schema';
Expand All @@ -30,20 +36,36 @@ import type {
} from '@o3r/styling';

/**
* SassCalculation interface
* This method will iterate on all characters in str starting from startingPosition and return the substring that is balanced.
* @param str
* @param startingPosition
*/
interface SassCalculation extends Value {
name: 'calc';
$arguments: string[];
}
const getBalancedString = (str: string, startingPosition = 0) => {
let nbToClose = 0;
for (let i = startingPosition; i < str.length; i++) {
const char = str[i];

Check warning on line 46 in packages/@o3r/styling/builders/style-extractor/helpers/css-variable.extractor.ts

View check run for this annotation

Codecov / codecov/patch

packages/@o3r/styling/builders/style-extractor/helpers/css-variable.extractor.ts#L44-L46

Added lines #L44 - L46 were not covered by tests
if (char === '(') {
nbToClose++;

Check warning on line 48 in packages/@o3r/styling/builders/style-extractor/helpers/css-variable.extractor.ts

View check run for this annotation

Codecov / codecov/patch

packages/@o3r/styling/builders/style-extractor/helpers/css-variable.extractor.ts#L48

Added line #L48 was not covered by tests
} else if (char === ')') {
nbToClose--;

Check warning on line 50 in packages/@o3r/styling/builders/style-extractor/helpers/css-variable.extractor.ts

View check run for this annotation

Codecov / codecov/patch

packages/@o3r/styling/builders/style-extractor/helpers/css-variable.extractor.ts#L50

Added line #L50 was not covered by tests

if (nbToClose === 0) {
return str.substring(startingPosition, i + 1);

Check warning on line 53 in packages/@o3r/styling/builders/style-extractor/helpers/css-variable.extractor.ts

View check run for this annotation

Codecov / codecov/patch

packages/@o3r/styling/builders/style-extractor/helpers/css-variable.extractor.ts#L53

Added line #L53 was not covered by tests
}
}
}
return str.substring(startingPosition);

Check warning on line 57 in packages/@o3r/styling/builders/style-extractor/helpers/css-variable.extractor.ts

View check run for this annotation

Codecov / codecov/patch

packages/@o3r/styling/builders/style-extractor/helpers/css-variable.extractor.ts#L57

Added line #L57 was not covered by tests
};

/**
* CSS Variable extractor
*/
export class CssVariableExtractor {
private static readonly asyncCompiler: Promise<AsyncCompiler> = initAsyncCompiler();
private static readonly varRegex = /var\(\s*--(.*?)\s*,\s*(.*)\)/;
private readonly cache: Record<string, URL> = {};

constructor(public defaultSassOptions?: StringOptions<'sync'>, private readonly builderOptions?: Pick<StyleExtractorBuilderSchema, 'ignoreInvalidValue'>) {}
constructor(public defaultSassOptions?: StringOptions<'async'>, private readonly builderOptions?: Pick<StyleExtractorBuilderSchema, 'ignoreInvalidValue'>) {}

/**
* Parse the CSS variable as reported
Expand All @@ -52,31 +74,29 @@ export class CssVariableExtractor {
*/
private parseCssVariable(name: string, value = ''): CssVariable {
const defaultValue = value.trim();
const res = defaultValue.match(/^var\(\s*([^\s),]+)\s*(?:,\s*([^(),]+(?:\([^)]*\))?))*\s*\)$/);

const ret: CssVariable = { name, defaultValue };
if (res) {
ret.references = [
this.parseCssVariable(res[1].replace(/^--/, ''), res[2])
];
} else {
let findRef = defaultValue;
let ref: RegExpExecArray | null;
const references: Record<string, CssVariable> = {};
do {
ref = /var\(\s*([^\s),]+)\s*(?:,\s*([^(),]+(\([^)]*\))?))*\s*\)/.exec(findRef);

if (ref) {
const refName = ref[1].replace(/^--/, '');
references[refName] = this.parseCssVariable(refName, ref[2]);
findRef = findRef.replace(ref[0], '');
const resultingCssVariable: CssVariable = { name, defaultValue };
let remainingValue = defaultValue;
let referenceMatch: RegExpExecArray | null;
const references: Record<string, CssVariable> = {};
do {
const varIndex = remainingValue.indexOf('var(');
if (varIndex === -1) {
// No more var() references
break;
} else {
const balancedVarDeclaration = getBalancedString(remainingValue, varIndex);
referenceMatch = CssVariableExtractor.varRegex.exec(balancedVarDeclaration);

Check warning on line 88 in packages/@o3r/styling/builders/style-extractor/helpers/css-variable.extractor.ts

View check run for this annotation

Codecov / codecov/patch

packages/@o3r/styling/builders/style-extractor/helpers/css-variable.extractor.ts#L87-L88

Added lines #L87 - L88 were not covered by tests
if (referenceMatch) {
const refName = referenceMatch[1];
references[refName] = this.parseCssVariable(refName, referenceMatch[2]);
remainingValue = remainingValue.replace(balancedVarDeclaration, '');

Check warning on line 92 in packages/@o3r/styling/builders/style-extractor/helpers/css-variable.extractor.ts

View check run for this annotation

Codecov / codecov/patch

packages/@o3r/styling/builders/style-extractor/helpers/css-variable.extractor.ts#L90-L92

Added lines #L90 - L92 were not covered by tests
}
} while (ref);
if (Object.keys(references).length > 0) {
ret.references = Object.values(references);
}
} while (referenceMatch);
if (Object.keys(references).length > 0) {
resultingCssVariable.references = Object.values(references);

Check warning on line 97 in packages/@o3r/styling/builders/style-extractor/helpers/css-variable.extractor.ts

View check run for this annotation

Codecov / codecov/patch

packages/@o3r/styling/builders/style-extractor/helpers/css-variable.extractor.ts#L97

Added line #L97 was not covered by tests
}
return ret;
return resultingCssVariable;
}

/**
Expand All @@ -92,7 +112,9 @@ export class CssVariableExtractor {
* @param color Sass Color
*/
private static getColorString(color: SassColor) {
return color.alpha ? `rgba(${color.red}, ${color.green}, ${color.blue}, ${color.alpha})` : `rgb(${color.red}, ${color.green}, ${color.blue}})`;
return color.alpha
? `rgba(${color.channel('red')}, ${color.channel('green')}, ${color.channel('blue')}, ${color.alpha})`
: `rgb(${color.channel('red')}, ${color.channel('green')}, ${color.channel('blue')}})`;
}

/**
Expand Down Expand Up @@ -127,16 +149,35 @@ export class CssVariableExtractor {
return contextTags;
}

private static getCalcString(item: CalculationValue, isSubCalc: boolean): string {

Check warning on line 152 in packages/@o3r/styling/builders/style-extractor/helpers/css-variable.extractor.ts

View check run for this annotation

Codecov / codecov/patch

packages/@o3r/styling/builders/style-extractor/helpers/css-variable.extractor.ts#L152

Added line #L152 was not covered by tests
if (item) {
if (item instanceof SassNumber) {
const value = item.value;

Check warning on line 155 in packages/@o3r/styling/builders/style-extractor/helpers/css-variable.extractor.ts

View check run for this annotation

Codecov / codecov/patch

packages/@o3r/styling/builders/style-extractor/helpers/css-variable.extractor.ts#L155

Added line #L155 was not covered by tests
const unit = item.numeratorUnits.get(0) ?? '';
return value + unit;

Check warning on line 157 in packages/@o3r/styling/builders/style-extractor/helpers/css-variable.extractor.ts

View check run for this annotation

Codecov / codecov/patch

packages/@o3r/styling/builders/style-extractor/helpers/css-variable.extractor.ts#L157

Added line #L157 was not covered by tests
} else if (item instanceof SassString) {
return item.text;

Check warning on line 159 in packages/@o3r/styling/builders/style-extractor/helpers/css-variable.extractor.ts

View check run for this annotation

Codecov / codecov/patch

packages/@o3r/styling/builders/style-extractor/helpers/css-variable.extractor.ts#L159

Added line #L159 was not covered by tests
} else if (item instanceof CalculationOperation) {
return `${isSubCalc ? '(' : ''}${CssVariableExtractor.getCalcString(item.left, true)} ${item.operator} ${CssVariableExtractor.getCalcString(item.right, true)}${isSubCalc ? ')' : ''}`;
} else if (item instanceof CalculationInterpolation) {
return item.value;

Check warning on line 163 in packages/@o3r/styling/builders/style-extractor/helpers/css-variable.extractor.ts

View check run for this annotation

Codecov / codecov/patch

packages/@o3r/styling/builders/style-extractor/helpers/css-variable.extractor.ts#L163

Added line #L163 was not covered by tests
} else {
return `calc(${item.arguments.toArray().map((arg) => CssVariableExtractor.getCalcString(arg, false)).join(' ')})`;

Check warning on line 165 in packages/@o3r/styling/builders/style-extractor/helpers/css-variable.extractor.ts

View check run for this annotation

Codecov / codecov/patch

packages/@o3r/styling/builders/style-extractor/helpers/css-variable.extractor.ts#L165

Added line #L165 was not covered by tests
}
}
return '';

Check warning on line 168 in packages/@o3r/styling/builders/style-extractor/helpers/css-variable.extractor.ts

View check run for this annotation

Codecov / codecov/patch

packages/@o3r/styling/builders/style-extractor/helpers/css-variable.extractor.ts#L168

Added line #L168 was not covered by tests
}

/**
* Extract metadata from Sass Content
* @param sassFilePath SCSS file URL
* @param sassFileContent SCSS file content
* @param additionalSassOptions
*/
public extractFileContent(sassFilePath: string, sassFileContent: string, additionalSassOptions?: StringOptions<'sync'>) {
public async extractFileContent(sassFilePath: string, sassFileContent: string, additionalSassOptions?: StringOptions<'async'>) {
const cssVariables: CssVariable[] = [];

const options: StringOptions<'sync'> = {
const options: StringOptions<'async'> = {
...this.defaultSassOptions,
...additionalSassOptions,
loadPaths: [path.dirname(sassFilePath)],
Expand Down Expand Up @@ -226,9 +267,8 @@ export class CssVariableExtractor {
if (!(varName instanceof SassString)) {
throw new O3rCliError('Invalid variable name');
}

let parsedValue: string | undefined;
if (varValue instanceof SassString || varValue instanceof SassNumber || varValue instanceof SassBoolean) {
if (varValue instanceof SassString || varValue instanceof SassNumber || varValue === sassTrue || varValue === sassFalse) {
parsedValue = varValue.toString();
} else if (varValue instanceof SassColor) {
parsedValue = CssVariableExtractor.getColorString(varValue);
Expand All @@ -237,12 +277,12 @@ export class CssVariableExtractor {
const parsedValueItems: string[] = [];
for (let i = 0; i < varValue.asList.size; i++) {
const item = varValue.get(i);
if (item instanceof SassString || item instanceof SassNumber || item instanceof SassBoolean) {
if (item instanceof SassString || item instanceof SassNumber || item === sassTrue || item === sassFalse) {
parsedValueItems.push(item.toString());
} else if (item instanceof SassColor) {
parsedValueItems.push(CssVariableExtractor.getColorString(item));
} else if (CssVariableExtractor.isSassCalculation(item)) {
parsedValueItems.push(`calc(${item.$arguments[0]})`);
parsedValueItems.push(`calc(${item.arguments.toArray().map((arg) => CssVariableExtractor.getCalcString(arg, false)).join(' ')})`);

Check warning on line 285 in packages/@o3r/styling/builders/style-extractor/helpers/css-variable.extractor.ts

View check run for this annotation

Codecov / codecov/patch

packages/@o3r/styling/builders/style-extractor/helpers/css-variable.extractor.ts#L285

Added line #L285 was not covered by tests
} else {
invalidIndexes.push(i);
}
Expand All @@ -258,7 +298,7 @@ export class CssVariableExtractor {
}
}
} else if (CssVariableExtractor.isSassCalculation(varValue)) {
parsedValue = `calc(${varValue.$arguments[0]})`;
parsedValue = `calc(${varValue.arguments.toArray().map((arg) => CssVariableExtractor.getCalcString(arg, false)).join(' ')})`;

Check warning on line 301 in packages/@o3r/styling/builders/style-extractor/helpers/css-variable.extractor.ts

View check run for this annotation

Codecov / codecov/patch

packages/@o3r/styling/builders/style-extractor/helpers/css-variable.extractor.ts#L301

Added line #L301 was not covered by tests
} else if (varValue.realNull) {
const message = `Invalid value for variable ${varName.text}.`;
if (this.builderOptions?.ignoreInvalidValue ?? true) {
Expand Down Expand Up @@ -305,17 +345,24 @@ export class CssVariableExtractor {
}
};

compileString(sassFileContent, options);
await (await CssVariableExtractor.asyncCompiler).compileStringAsync(sassFileContent, options);
return cssVariables;
}

/**
* Dispose the async compiler. Must be called once when extraction is done.
*/
public async disposeAsyncCompiler() {
await (await CssVariableExtractor.asyncCompiler).dispose();
}

/**
* Extract metadata from Sass file
* @param sassFilePath SCSS file to parse
*/
public extractFile(sassFilePath: string): CssVariable[] {
public async extractFile(sassFilePath: string): Promise<CssVariable[]> {

Check warning on line 363 in packages/@o3r/styling/builders/style-extractor/helpers/css-variable.extractor.ts

View check run for this annotation

Codecov / codecov/patch

packages/@o3r/styling/builders/style-extractor/helpers/css-variable.extractor.ts#L363

Added line #L363 was not covered by tests
const sassFileContent = fs.readFileSync(sassFilePath, { encoding: 'utf8' });
return this.extractFileContent(sassFilePath, sassFileContent);
return await this.extractFileContent(sassFilePath, sassFileContent);

Check warning on line 365 in packages/@o3r/styling/builders/style-extractor/helpers/css-variable.extractor.ts

View check run for this annotation

Codecov / codecov/patch

packages/@o3r/styling/builders/style-extractor/helpers/css-variable.extractor.ts#L365

Added line #L365 was not covered by tests
}

/**
Expand All @@ -327,10 +374,7 @@ export class CssVariableExtractor {
return libraries
.map((lib) => getLibraryCmsMetadata(lib))
.filter(({ styleFilePath }) => !!styleFilePath)
.map(({ styleFilePath }) => {
const libConfig = JSON.parse(fs.readFileSync(styleFilePath!, 'utf8'));
return libConfig as CssMetadata;
})
.map(({ styleFilePath }) => JSON.parse(fs.readFileSync(styleFilePath!, 'utf8')) as CssMetadata)

Check warning on line 377 in packages/@o3r/styling/builders/style-extractor/helpers/css-variable.extractor.ts

View check run for this annotation

Codecov / codecov/patch

packages/@o3r/styling/builders/style-extractor/helpers/css-variable.extractor.ts#L377

Added line #L377 was not covered by tests
.reduce<CssMetadata>((acc, libMetadata) => {
return Object.keys(libMetadata.variables)
.filter((key) => !!acc.variables[key])
Expand Down
34 changes: 17 additions & 17 deletions packages/@o3r/styling/builders/style-extractor/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import {
} from 'globby';
import type {
Logger,
} from 'sass';
} from 'sass-embedded';
import * as ts from 'typescript';
import {
CssVariableExtractor,
Expand Down Expand Up @@ -76,10 +76,10 @@ export default createBuilder(createBuilderWithMetricsIfInstalled<StyleExtractorB
/** CSS Metadata file to write */
let cssMetadata = (
// extract metadata for each file
await Promise.all(files.map((file, idx) => {
await Promise.all(files.map(async (file, idx) => {

Check warning on line 79 in packages/@o3r/styling/builders/style-extractor/index.ts

View check run for this annotation

Codecov / codecov/patch

packages/@o3r/styling/builders/style-extractor/index.ts#L79

Added line #L79 was not covered by tests
try {
context.reportProgress(idx, STEP_NUMBER, `Extracting ${file}`);
const variables = cssVariableExtractor.extractFile(file);
const variables = await cssVariableExtractor.extractFile(file);

Check warning on line 82 in packages/@o3r/styling/builders/style-extractor/index.ts

View check run for this annotation

Codecov / codecov/patch

packages/@o3r/styling/builders/style-extractor/index.ts#L82

Added line #L82 was not covered by tests
const themeFileSuffix = '.style.theme.scss';
if (file.endsWith(themeFileSuffix)) {
const componentPath = path.join(path.dirname(file), `${path.basename(file, themeFileSuffix)}.component.ts`);
Expand All @@ -92,7 +92,10 @@ export default createBuilder(createBuilderWithMetricsIfInstalled<StyleExtractorB
const componentDeclaration = componentSourceFile.statements.find((s): s is ts.ClassDeclaration => ts.isClassDeclaration(s) && isO3rClassComponent(s));
const componentName: string | undefined = componentDeclaration?.name?.escapedText.toString();
if (componentName) {
return variables.map((variable) => ({ ...variable, component: { library: libraryName, name: componentName } }));
for (const variable of variables) {
variable.component = { library: libraryName, name: componentName };

Check warning on line 96 in packages/@o3r/styling/builders/style-extractor/index.ts

View check run for this annotation

Codecov / codecov/patch

packages/@o3r/styling/builders/style-extractor/index.ts#L95-L96

Added lines #L95 - L96 were not covered by tests
}
return variables;

Check warning on line 98 in packages/@o3r/styling/builders/style-extractor/index.ts

View check run for this annotation

Codecov / codecov/patch

packages/@o3r/styling/builders/style-extractor/index.ts#L98

Added line #L98 was not covered by tests
}
}
return variables;
Expand All @@ -102,19 +105,13 @@ export default createBuilder(createBuilderWithMetricsIfInstalled<StyleExtractorB
}
}))
).reduce<CssMetadata>((acc, cssVarList) => {
// Check duplicate CSS variable
cssVarList
.filter((cssVar) => !!acc.variables[cssVar.name])
.filter((cssVar) => !initialPreviousMetadata.variables[cssVar.name] && acc.variables[cssVar.name].defaultValue !== cssVar.defaultValue)
.forEach((cssVar) =>
context.logger[options.ignoreDuplicateWarning ? 'debug' : 'warn'](`Duplicate "${cssVar.name}" (${acc.variables[cssVar.name].defaultValue} will be replaced by ${cssVar.defaultValue})`)
);
for (const cssVar of cssVarList) {

Check warning on line 108 in packages/@o3r/styling/builders/style-extractor/index.ts

View check run for this annotation

Codecov / codecov/patch

packages/@o3r/styling/builders/style-extractor/index.ts#L108

Added line #L108 was not covered by tests
if (!!acc.variables[cssVar.name] && !initialPreviousMetadata.variables[cssVar.name] && acc.variables[cssVar.name].defaultValue !== cssVar.defaultValue) {
context.logger[options.ignoreDuplicateWarning ? 'debug' : 'warn'](`Duplicate "${cssVar.name}" (${acc.variables[cssVar.name].defaultValue} will be replaced by ${cssVar.defaultValue})`);
}
acc.variables[cssVar.name] = cssVar;

Check warning on line 112 in packages/@o3r/styling/builders/style-extractor/index.ts

View check run for this annotation

Codecov / codecov/patch

packages/@o3r/styling/builders/style-extractor/index.ts#L112

Added line #L112 was not covered by tests
}

// merge all variables form all the files
cssVarList
.forEach((item) => {
acc.variables[item.name] = item;
});
return acc;
}, initialPreviousMetadata);

Expand Down Expand Up @@ -235,6 +232,7 @@ export default createBuilder(createBuilderWithMetricsIfInstalled<StyleExtractorB
});

context.addTeardown(async () => {
await cssVariableExtractor.disposeAsyncCompiler();

Check warning on line 235 in packages/@o3r/styling/builders/style-extractor/index.ts

View check run for this annotation

Codecov / codecov/patch

packages/@o3r/styling/builders/style-extractor/index.ts#L235

Added line #L235 was not covered by tests
await watcher.close();
await metadataWatcher.close();
});
Expand All @@ -245,6 +243,8 @@ export default createBuilder(createBuilderWithMetricsIfInstalled<StyleExtractorB
.on('error', (err) => reject(err))
);
} else {
return execute(getAllFiles());
const result = execute(getAllFiles());
void cssVariableExtractor.disposeAsyncCompiler();
return result;
}
}));
Loading

0 comments on commit c24ca1e

Please sign in to comment.