Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "patch",
"comment": "feat: add validation into mergeClasses for makeResetStyles calls",
"packageName": "@griffel/core",
"email": "[email protected]",
"dependentChangeType": "patch"
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "patch",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't this be a minor ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup

"comment": "feat: add support for makeResetStyles",
"packageName": "@griffel/jest-serializer",
"email": "[email protected]",
"dependentChangeType": "patch"
}
8 changes: 7 additions & 1 deletion packages/core/src/__resetCSS.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { DEBUG_RESET_CLASSES } from './constants';
import type { MakeStylesOptions } from './types';

/**
Expand All @@ -6,8 +7,13 @@ import type { MakeStylesOptions } from './types';
export function __resetCSS(ltrClassName: string, rtlClassName: string | null) {
function computeClassName(options: Pick<MakeStylesOptions, 'dir'>): string {
const { dir } = options;
const className = dir === 'ltr' ? ltrClassName : rtlClassName || ltrClassName;

return dir === 'ltr' ? ltrClassName : rtlClassName || ltrClassName;
if (process.env.NODE_ENV !== 'production') {
DEBUG_RESET_CLASSES[className] = 1;
}

return className;
}

return computeClassName;
Expand Down
9 changes: 8 additions & 1 deletion packages/core/src/__resetStyles.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { DEBUG_RESET_CLASSES } from './constants';
import type { MakeStylesOptions } from './types';

/**
Expand All @@ -18,7 +19,13 @@ export function __resetStyles(ltrClassName: string, rtlClassName: string | null,
insertionCache[rendererId] = true;
}

return isLTR ? ltrClassName : rtlClassName || ltrClassName;
const className = isLTR ? ltrClassName : rtlClassName || ltrClassName;

if (process.env.NODE_ENV !== 'production') {
DEBUG_RESET_CLASSES[className] = 1;
}

return className;
}

return computeClassName;
Expand Down
3 changes: 3 additions & 0 deletions packages/core/src/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,9 @@ export const SEQUENCE_SIZE =
? SEQUENCE_PREFIX.length + SEQUENCE_HASH_LENGTH
: SEQUENCE_PREFIX.length + SEQUENCE_HASH_LENGTH + DEBUG_SEQUENCE_SEPARATOR.length + SEQUENCE_HASH_LENGTH;

/** @internal */
export const DEBUG_RESET_CLASSES: Record<string, 1> = {};

/** @internal */
export const DEFINITION_LOOKUP_TABLE: Record<SequenceHash, LookupItem> = {};

Expand Down
12 changes: 8 additions & 4 deletions packages/core/src/makeResetStyles.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,7 @@
import { DEBUG_RESET_CLASSES } from './constants';
import { resolveResetStyleRules } from './runtime/resolveResetStyleRules';
import type { GriffelResetStyle, MakeStylesOptions } from './types';

/**
* @internal
*/
Comment on lines -4 to -6
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, a leftover from the time when it was unstable.

export function makeResetStyles(styles: GriffelResetStyle) {
const insertionCache: Record<string, boolean> = {};

Expand All @@ -28,7 +26,13 @@ export function makeResetStyles(styles: GriffelResetStyle) {
insertionCache[rendererId] = true;
}

return isLTR ? ltrClassName : rtlClassName || ltrClassName;
const className = isLTR ? ltrClassName : rtlClassName || ltrClassName;

if (process.env.NODE_ENV !== 'production') {
DEBUG_RESET_CLASSES[className] = 1;
}

return className;
}

return computeClassName;
Expand Down
14 changes: 14 additions & 0 deletions packages/core/src/mergeClasses.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { makeStyles } from './makeStyles';
import { createDOMRenderer } from './renderer/createDOMRenderer';
import { MakeStylesOptions } from './types';
import { SEQUENCE_PREFIX, SEQUENCE_SIZE } from './constants';
import { makeResetStyles } from './makeResetStyles';

function removeSequenceHash(classNames: string) {
const indexOfSequence = classNames.indexOf(SEQUENCE_PREFIX);
Expand Down Expand Up @@ -149,6 +150,19 @@ describe('mergeClasses', () => {
expect(error).toHaveBeenCalledWith(expect.stringMatching(/that has different direction \(dir="rtl"\)/));
});

it('warns if contains multiple classes from makeResetStyles', () => {
// eslint-disable-next-line @typescript-eslint/no-empty-function
const error = jest.spyOn(console, 'error').mockImplementationOnce(() => {});

const className1 = makeResetStyles({ display: 'block' })(options);
const className2 = makeResetStyles({ display: 'grid' })(options);

expect(mergeClasses(className1, className2)).toMatchInlineSnapshot(`r13o7eu2 rlgj0h8`);
expect(error).toHaveBeenCalledWith(
expect.stringMatching(/a passed string contains multiple classes produced by makeResetStyles/),
);
});

describe('"dir" option', () => {
it('performs deduplication for RTL classes', () => {
const computeClasses = makeStyles({
Expand Down
23 changes: 23 additions & 0 deletions packages/core/src/mergeClasses.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
import {
DEBUG_RESET_CLASSES,
DEFINITION_LOOKUP_TABLE,
LOOKUP_DEFINITIONS_INDEX,
LOOKUP_DIR_INDEX,
RESET_HASH_PREFIX,
SEQUENCE_PREFIX,
SEQUENCE_SIZE,
} from './constants';
Expand Down Expand Up @@ -43,6 +45,8 @@ export function mergeClasses(): string {
let sequenceMatch = '';
const sequencesIds: (SequenceHash | undefined)[] = new Array(arguments.length);

let containsResetClassName = '';

for (let i = 0; i < arguments.length; i++) {
const className = arguments[i];

Expand All @@ -52,6 +56,25 @@ export function mergeClasses(): string {
const sequenceIndex = className.indexOf(SEQUENCE_PREFIX);

if (sequenceIndex === -1) {
if (process.env.NODE_ENV !== 'production') {
className.split(' ').forEach(entry => {
if (entry.startsWith(RESET_HASH_PREFIX) && DEBUG_RESET_CLASSES[entry]) {
if (containsResetClassName) {
// eslint-disable-next-line no-console
console.error(
'mergeClasses(): a passed string contains multiple classes produced by makeResetStyles (' +
`${className} & ${resultClassName}, this will lead to non-deterministic behavior. Learn more:` +
'https://griffel.js.org/react/api/make-reset-styles#limitations' +
'\n' +
`Source string: ${className}`,
);
} else {
containsResetClassName = entry;
}
}
});
}

resultClassName += className + ' ';
} else {
const sequenceId = className.substr(sequenceIndex, SEQUENCE_SIZE);
Expand Down
40 changes: 39 additions & 1 deletion packages/jest-serializer/src/index.test.tsx
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import * as React from 'react';
import { makeStyles, mergeClasses, TextDirectionProvider } from '@griffel/react';
import { makeResetStyles, makeStyles, mergeClasses, TextDirectionProvider } from '@griffel/react';
import { render } from '@testing-library/react';

import { print, test } from './index';
Expand All @@ -19,6 +19,11 @@ const useStyles3 = makeStyles({
display: { display: 'none' },
});

const useBaseStyles = makeResetStyles({
color: 'red',
marginLeft: '20px',
});

const TestComponent: React.FC<{ id?: string }> = ({ id }) => {
const styles1 = useStyles1();
const styles2 = useStyles2();
Expand All @@ -28,6 +33,15 @@ const TestComponent: React.FC<{ id?: string }> = ({ id }) => {
return <div data-testid={id} className={styles} />;
};

const TestResetComponent: React.FC<{ id?: string }> = ({ id }) => {
const classes = useStyles1();
const baseClassName = useBaseStyles();

const className = mergeClasses('static-class', classes.paddingLeft, baseClassName);

return <div data-testid={id} className={className} />;
};

const RtlWrapper: React.FC = ({ children }) => <TextDirectionProvider dir="rtl">{children}</TextDirectionProvider>;

describe('serializer', () => {
Expand All @@ -37,11 +51,24 @@ describe('serializer', () => {
paddingLeft: '10px',
paddingRight: '20px',
});
expect(render(<TestResetComponent id="reset-test" />).getByTestId('reset-test')).toHaveStyle({
color: 'red',
paddingLeft: '10px',
marginLeft: '20px',
});

expect(render(<TestComponent id="rtl-test" />, { wrapper: RtlWrapper }).getByTestId('rtl-test')).toHaveStyle({
display: 'none',
paddingLeft: '20px',
paddingRight: '10px',
});
expect(
render(<TestResetComponent id="rtl-reset-test" />, { wrapper: RtlWrapper }).getByTestId('rtl-reset-test'),
).toHaveStyle({
color: 'red',
paddingRight: '10px',
marginRight: '20px',
});
});

it('renders without generated classes', () => {
Expand All @@ -50,10 +77,21 @@ describe('serializer', () => {
class="static-class"
/>
`);
expect(render(<TestResetComponent />).container.firstChild).toMatchInlineSnapshot(`
<div
class="static-class"
/>
`);

expect(render(<TestComponent />, { wrapper: RtlWrapper }).container.firstChild).toMatchInlineSnapshot(`
<div
class="static-class"
/>
`);
expect(render(<TestResetComponent />, { wrapper: RtlWrapper }).container.firstChild).toMatchInlineSnapshot(`
<div
class="static-class"
/>
`);
});
});
12 changes: 9 additions & 3 deletions packages/jest-serializer/src/index.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { DEFINITION_LOOKUP_TABLE, CSSClasses } from '@griffel/core';
import { DEBUG_RESET_CLASSES, DEFINITION_LOOKUP_TABLE, CSSClasses } from '@griffel/core';

export function print(val: unknown) {
/**
Expand All @@ -17,6 +17,12 @@ export function print(val: unknown) {

while ((result = regex.exec(_val))) {
const [name] = result;

if (DEBUG_RESET_CLASSES[name]) {
regexParts.push(name);
continue;
}

const [definitions] = DEFINITION_LOOKUP_TABLE[name];

/**
Expand All @@ -34,7 +40,7 @@ export function print(val: unknown) {
/**
* form parts of regular expression and removes collected classNames from string
* @example
* regex = /r?(f16th3vw|frdkuqy0|fat0sn40|fjseox00)/
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't use r prefix for RTL classes for a long time.

* regex = /f16th3vw|frdkuqy0|fat0sn40|fjseox00/
*/
const valStrippedClassNames = _val.replace(new RegExp(regexParts.join('|'), 'g'), '').trim();

Expand All @@ -61,7 +67,7 @@ export function test(val: unknown) {
* lookupRegex() // /(__1qdh4ig)/g
*/
function lookupRegex(): RegExp | undefined {
const definitionKeys = Object.keys(DEFINITION_LOOKUP_TABLE);
const definitionKeys = Object.keys({ ...DEFINITION_LOOKUP_TABLE, ...DEBUG_RESET_CLASSES });

if (definitionKeys.length) {
return new RegExp(`${definitionKeys.join('|')}`, 'g');
Expand Down