From d8cc0a0c61ece1443b591b238e52446e4a78eec0 Mon Sep 17 00:00:00 2001 From: Pavel Klibani Date: Wed, 18 Dec 2024 11:03:37 +0100 Subject: [PATCH 1/3] UNSAFE classname test --- examples/next-with-app-router/package.json | 3 +- .../next-with-app-router/src/app/globals.scss | 12 ++++ .../app/test1/components/RandomComponent.tsx | 9 +++ .../app/test1/components/TestComponent.tsx | 30 +++++++++ .../src/app/test1/components/newStyleProps.ts | 67 +++++++++++++++++++ .../test1/components/originalStyleProps.ts | 57 ++++++++++++++++ .../src/app/test1/page.tsx | 58 ++++++++++++++++ yarn.lock | 29 +++++++- 8 files changed, 263 insertions(+), 2 deletions(-) create mode 100644 examples/next-with-app-router/src/app/test1/components/RandomComponent.tsx create mode 100644 examples/next-with-app-router/src/app/test1/components/TestComponent.tsx create mode 100644 examples/next-with-app-router/src/app/test1/components/newStyleProps.ts create mode 100644 examples/next-with-app-router/src/app/test1/components/originalStyleProps.ts create mode 100644 examples/next-with-app-router/src/app/test1/page.tsx diff --git a/examples/next-with-app-router/package.json b/examples/next-with-app-router/package.json index 8e6d83b3ed..a78b0f988d 100644 --- a/examples/next-with-app-router/package.json +++ b/examples/next-with-app-router/package.json @@ -15,7 +15,8 @@ "@lmc-eu/spirit-web-react": "workspace:^", "next": "14.2.18", "react": "^18", - "react-dom": "^18" + "react-dom": "^18", + "sass": "^1.83.0" }, "devDependencies": { "@next/eslint-plugin-next": "14.2.18", diff --git a/examples/next-with-app-router/src/app/globals.scss b/examples/next-with-app-router/src/app/globals.scss index b2ee58bb42..dc9f6936ec 100644 --- a/examples/next-with-app-router/src/app/globals.scss +++ b/examples/next-with-app-router/src/app/globals.scss @@ -1 +1,13 @@ @import '@lmc-eu/spirit-web/src/scss'; + +.test { + font-weight: bold; + + &::before { + content: '✅ '; + } +} + +p:not(.test)::before { + content: '❌ '; +} diff --git a/examples/next-with-app-router/src/app/test1/components/RandomComponent.tsx b/examples/next-with-app-router/src/app/test1/components/RandomComponent.tsx new file mode 100644 index 0000000000..151b0a54b7 --- /dev/null +++ b/examples/next-with-app-router/src/app/test1/components/RandomComponent.tsx @@ -0,0 +1,9 @@ +'use client'; + +import React from 'react'; + +const RandomComponent = (props: any) => { + return

{props.children}

; +}; + +export default RandomComponent; diff --git a/examples/next-with-app-router/src/app/test1/components/TestComponent.tsx b/examples/next-with-app-router/src/app/test1/components/TestComponent.tsx new file mode 100644 index 0000000000..0b89e1855f --- /dev/null +++ b/examples/next-with-app-router/src/app/test1/components/TestComponent.tsx @@ -0,0 +1,30 @@ +'use client'; + +import React, { ElementType, ReactNode } from 'react'; +import { StyleProps, TransferProps } from '../../../../../../packages/web-react/src/types'; +// import { useStyleProps } from './originalStyleProps'; +import { useStyleProps } from './newStyleProps'; + +interface TestComponentProps extends StyleProps, TransferProps { + elementType?: ElementType | string; + children?: string | ReactNode; +} + +const defaultProps: TestComponentProps = { + elementType: 'p', +}; + +const TestComponent = (props: TestComponentProps) => { + const propsWithDefaults = { ...defaultProps, ...props }; + const { elementType: ElementTag = 'p', children, ...rest } = propsWithDefaults; + + const { styleProps, props: transferProps } = useStyleProps({ ElementTag, ...rest }); + + return ( + + {children} + + ); +}; + +export default TestComponent; diff --git a/examples/next-with-app-router/src/app/test1/components/newStyleProps.ts b/examples/next-with-app-router/src/app/test1/components/newStyleProps.ts new file mode 100644 index 0000000000..2c822c0afa --- /dev/null +++ b/examples/next-with-app-router/src/app/test1/components/newStyleProps.ts @@ -0,0 +1,67 @@ +import classNames from 'classnames'; +import { CSSProperties, HTMLAttributes, useContext } from 'react'; +import { warning } from '../../../../../../packages/web-react/src//common/utilities'; +import ClassNamePrefixContext from '../../../../../../packages/web-react/src/context/ClassNamePrefixContext'; +import { StyleProps } from '../../../../../../packages/web-react/src//types'; +import { useStyleUtilities } from '../../../../../../packages/web-react/src/hooks/useStyleUtilities'; + +export type StylePropsResult = { + styleProps: HTMLAttributes; + props: HTMLAttributes; +}; + +export function useStyleProps(props: T): StylePropsResult { + const classNamePrefix = useContext(ClassNamePrefixContext); + const { UNSAFE_className, UNSAFE_style, ElementTag, ...otherProps } = props; + const { styleUtilities, props: modifiedProps } = useStyleUtilities(otherProps, classNamePrefix); + + const style: CSSProperties = { ...UNSAFE_style }; + + if (typeof ElementTag !== 'function') { + // Want to check if className prop exists, but not to define it in StyleProps type + // @ts-expect-error Property 'className' does not exist on type 'Omit'. + if (modifiedProps.className) { + warning( + false, + 'The className prop is unsafe and is unsupported in Spirit Web React. ' + + 'Please use style props with Spirit Design Tokens, or UNSAFE_className if you absolutely must do something custom. ' + + 'Note that this may break in future versions due to DOM structure changes.', + ); + + // @ts-expect-error same as above, let me live my life + delete modifiedProps.className; + } + + // Want to check if style prop exists, but not to define it in StyleProps type + // @ts-expect-error Property 'style' does not exist on type 'Omit'. + if (modifiedProps.style) { + warning( + false, + 'The style prop is unsafe and is unsupported in Spirit Web React. ' + + 'Please use style props with Spirit Design Tokens, or UNSAFE_style if you absolutely must do something custom. ' + + 'Note that this may break in future versions due to DOM structure changes.', + ); + + // @ts-expect-error same as above, let me live my life + delete modifiedProps.style; + } + + const styleProps = { + style: Object.keys(style).length > 0 ? style : undefined, + className: classNames(UNSAFE_className, ...styleUtilities) || undefined, + }; + + return { + styleProps, + props: modifiedProps as HTMLAttributes, + }; + } + + return { + styleProps: { + ...(UNSAFE_style !== undefined && { UNSAFE_style }), + ...(UNSAFE_className !== undefined && { UNSAFE_className }), + }, + props: { ...modifiedProps }, + }; +} diff --git a/examples/next-with-app-router/src/app/test1/components/originalStyleProps.ts b/examples/next-with-app-router/src/app/test1/components/originalStyleProps.ts new file mode 100644 index 0000000000..a41b4eb57b --- /dev/null +++ b/examples/next-with-app-router/src/app/test1/components/originalStyleProps.ts @@ -0,0 +1,57 @@ +import classNames from 'classnames'; +import { CSSProperties, HTMLAttributes, useContext } from 'react'; +import { warning } from '../../../../../../packages/web-react/src//common/utilities'; +import ClassNamePrefixContext from '../../../../../../packages/web-react/src/context/ClassNamePrefixContext'; +import { StyleProps } from '../../../../../../packages/web-react/src//types'; +import { useStyleUtilities } from '../../../../../../packages/web-react/src/hooks/useStyleUtilities'; + +export type StylePropsResult = { + styleProps: HTMLAttributes; + props: HTMLAttributes; +}; + +export function useStyleProps(props: T): StylePropsResult { + const classNamePrefix = useContext(ClassNamePrefixContext); + const { UNSAFE_className, UNSAFE_style, ...otherProps } = props; + const { styleUtilities, props: modifiedProps } = useStyleUtilities(otherProps, classNamePrefix); + + const style: CSSProperties = { ...UNSAFE_style }; + + // Want to check if className prop exists, but not to define it in StyleProps type + // @ts-expect-error Property 'className' does not exist on type 'Omit'. + if (modifiedProps.className) { + warning( + false, + 'The className prop is unsafe and is unsupported in Spirit Web React. ' + + 'Please use style props with Spirit Design Tokens, or UNSAFE_className if you absolutely must do something custom. ' + + 'Note that this may break in future versions due to DOM structure changes.', + ); + + // @ts-expect-error same as above, let me live my life + delete modifiedProps.className; + } + + // Want to check if style prop exists, but not to define it in StyleProps type + // @ts-expect-error Property 'style' does not exist on type 'Omit'. + if (modifiedProps.style) { + warning( + false, + 'The style prop is unsafe and is unsupported in Spirit Web React. ' + + 'Please use style props with Spirit Design Tokens, or UNSAFE_style if you absolutely must do something custom. ' + + 'Note that this may break in future versions due to DOM structure changes.', + ); + + // @ts-expect-error same as above, let me live my life + delete modifiedProps.style; + } + + const styleProps = { + style: Object.keys(style).length > 0 ? style : undefined, + className: classNames(UNSAFE_className, ...styleUtilities) || undefined, + }; + + return { + styleProps, + props: modifiedProps as HTMLAttributes, + }; +} diff --git a/examples/next-with-app-router/src/app/test1/page.tsx b/examples/next-with-app-router/src/app/test1/page.tsx new file mode 100644 index 0000000000..4221126595 --- /dev/null +++ b/examples/next-with-app-router/src/app/test1/page.tsx @@ -0,0 +1,58 @@ +import React from 'react'; +import { Container, Heading, Text as SpiritText } from '@lmc-eu/spirit-web-react'; +import TestComponent from './components/TestComponent'; +import RandomComponent from './components/RandomComponent'; + +const page = () => { + return ( + + + Test component 1 + + + + Should work + + + {/* TESTING ⬇ */} + <> + + Spirit Text component without elementType + + + + HTML element "p" with UNSAFE class and style + + + + Spirit Text component with UNSAFE class and style + + + + Random component with class and style + + + {/* TESTING ⬆ */} + + + Should not work + + + {/* TESTING ⬇ */} + <> + {/* This will not be working - Spirit component accepts only UNSAFE style props */} + + Spirit Text component with class and style + + + {/* This will not be working - Random component doesn't know what UNSAFE style props are */} + + Random component with UNSAFE class and style + + + {/* TESTING ⬆ */} + + ); +}; + +export default page; diff --git a/yarn.lock b/yarn.lock index 837ef8c1be..292417641f 100644 --- a/yarn.lock +++ b/yarn.lock @@ -40,6 +40,7 @@ __metadata: next: "npm:14.2.18" react: "npm:^18" react-dom: "npm:^18" + sass: "npm:^1.83.0" typescript: "npm:5.6.3" languageName: unknown linkType: soft @@ -11790,6 +11791,15 @@ __metadata: languageName: node linkType: hard +"chokidar@npm:^4.0.0": + version: 4.0.2 + resolution: "chokidar@npm:4.0.2" + dependencies: + readdirp: "npm:^4.0.1" + checksum: 10/fc25d20d72ee0e74b5be1fd9df366dc8aa17709a59c364c321b6f35b6d2fd8c65d01bda74eb42ffd61ad7807e5de5e673c6bd503c2ed0ab2a79be5cb51d4c259 + languageName: node + linkType: hard + "chokidar@npm:^4.0.1": version: 4.0.1 resolution: "chokidar@npm:4.0.1" @@ -27265,6 +27275,23 @@ __metadata: languageName: node linkType: hard +"sass@npm:^1.83.0": + version: 1.83.0 + resolution: "sass@npm:1.83.0" + dependencies: + "@parcel/watcher": "npm:^2.4.1" + chokidar: "npm:^4.0.0" + immutable: "npm:^5.0.2" + source-map-js: "npm:>=0.6.2 <2.0.0" + dependenciesMeta: + "@parcel/watcher": + optional: true + bin: + sass: sass.js + checksum: 10/cae7c489ffeb1324ac7e766dda60206a6d7a318d0689b490290a32a6414ef1fd0f376f92d45fb1610e507baa6f6594f1a61d4d706df2f105122ff9a83d2a28e1 + languageName: node + linkType: hard + "sax@npm:~1.2.4": version: 1.2.4 resolution: "sax@npm:1.2.4" @@ -27808,7 +27835,7 @@ __metadata: languageName: node linkType: hard -"source-map-js@npm:^1.0.1, source-map-js@npm:^1.0.2, source-map-js@npm:^1.2.0, source-map-js@npm:^1.2.1": +"source-map-js@npm:>=0.6.2 <2.0.0, source-map-js@npm:^1.0.1, source-map-js@npm:^1.0.2, source-map-js@npm:^1.2.0, source-map-js@npm:^1.2.1": version: 1.2.1 resolution: "source-map-js@npm:1.2.1" checksum: 10/ff9d8c8bf096d534a5b7707e0382ef827b4dd360a577d3f34d2b9f48e12c9d230b5747974ee7c607f0df65113732711bb701fe9ece3c7edbd43cb2294d707df3 From ca896431dfa974fc6b2337057359701b30a0a6e6 Mon Sep 17 00:00:00 2001 From: Pavel Klibani Date: Wed, 18 Dec 2024 15:51:13 +0100 Subject: [PATCH 2/3] fixup! UNSAFE classname test --- .../{RandomComponent.tsx => OutsideComponent.tsx} | 4 ++-- .../src/app/test1/components/TestComponent.tsx | 1 + examples/next-with-app-router/src/app/test1/page.tsx | 6 +++--- 3 files changed, 6 insertions(+), 5 deletions(-) rename examples/next-with-app-router/src/app/test1/components/{RandomComponent.tsx => OutsideComponent.tsx} (54%) diff --git a/examples/next-with-app-router/src/app/test1/components/RandomComponent.tsx b/examples/next-with-app-router/src/app/test1/components/OutsideComponent.tsx similarity index 54% rename from examples/next-with-app-router/src/app/test1/components/RandomComponent.tsx rename to examples/next-with-app-router/src/app/test1/components/OutsideComponent.tsx index 151b0a54b7..c0a17f09a0 100644 --- a/examples/next-with-app-router/src/app/test1/components/RandomComponent.tsx +++ b/examples/next-with-app-router/src/app/test1/components/OutsideComponent.tsx @@ -2,8 +2,8 @@ import React from 'react'; -const RandomComponent = (props: any) => { +const OutsideComponent = (props: any) => { return

{props.children}

; }; -export default RandomComponent; +export default OutsideComponent; diff --git a/examples/next-with-app-router/src/app/test1/components/TestComponent.tsx b/examples/next-with-app-router/src/app/test1/components/TestComponent.tsx index 0b89e1855f..aa66448d6b 100644 --- a/examples/next-with-app-router/src/app/test1/components/TestComponent.tsx +++ b/examples/next-with-app-router/src/app/test1/components/TestComponent.tsx @@ -4,6 +4,7 @@ import React, { ElementType, ReactNode } from 'react'; import { StyleProps, TransferProps } from '../../../../../../packages/web-react/src/types'; // import { useStyleProps } from './originalStyleProps'; import { useStyleProps } from './newStyleProps'; +import classNames from 'classnames'; interface TestComponentProps extends StyleProps, TransferProps { elementType?: ElementType | string; diff --git a/examples/next-with-app-router/src/app/test1/page.tsx b/examples/next-with-app-router/src/app/test1/page.tsx index 4221126595..aff6f03dbc 100644 --- a/examples/next-with-app-router/src/app/test1/page.tsx +++ b/examples/next-with-app-router/src/app/test1/page.tsx @@ -1,7 +1,7 @@ import React from 'react'; import { Container, Heading, Text as SpiritText } from '@lmc-eu/spirit-web-react'; import TestComponent from './components/TestComponent'; -import RandomComponent from './components/RandomComponent'; +import OutsideComponent from './components/OutsideComponent'; const page = () => { return ( @@ -28,7 +28,7 @@ const page = () => { Spirit Text component with UNSAFE class and style - + Random component with class and style @@ -46,7 +46,7 @@ const page = () => { {/* This will not be working - Random component doesn't know what UNSAFE style props are */} - + Random component with UNSAFE class and style From d2025c1043b7ad6b593587576aa6b463f65a19d8 Mon Sep 17 00:00:00 2001 From: Pavel Klibani Date: Thu, 2 Jan 2025 11:02:49 +0100 Subject: [PATCH 3/3] Refactor(web-react): Apply condition to styleProps --- .../src/components/Tooltip/TooltipTrigger.tsx | 10 +-- packages/web-react/src/hooks/styleProps.ts | 83 +++++++++++-------- packages/web-react/src/types/shared/style.ts | 5 +- 3 files changed, 57 insertions(+), 41 deletions(-) diff --git a/packages/web-react/src/components/Tooltip/TooltipTrigger.tsx b/packages/web-react/src/components/Tooltip/TooltipTrigger.tsx index b8b86e98f9..b5ad2b4dcf 100644 --- a/packages/web-react/src/components/Tooltip/TooltipTrigger.tsx +++ b/packages/web-react/src/components/Tooltip/TooltipTrigger.tsx @@ -17,17 +17,15 @@ const defaultProps: TooltipTriggerProps = { const TooltipTrigger = (props: TooltipTriggerProps) => { const propsWithDefaults = { ...defaultProps, ...props }; - const { elementType = 'button', children, ...rest } = propsWithDefaults; + const { elementType: ElementTag = 'button', children, ...rest } = propsWithDefaults; const { id, isOpen, triggerRef, getReferenceProps } = useTooltipContext(); - const Component = elementType; - - const { styleProps: triggerStyleProps, props: transferProps } = useStyleProps(rest); + const { styleProps: triggerStyleProps, props: transferProps } = useStyleProps({ ElementTag, ...rest }); return ( - + {typeof children === 'function' ? children({ isOpen }) : children} - + ); }; diff --git a/packages/web-react/src/hooks/styleProps.ts b/packages/web-react/src/hooks/styleProps.ts index 5c533973f9..cef74cc573 100644 --- a/packages/web-react/src/hooks/styleProps.ts +++ b/packages/web-react/src/hooks/styleProps.ts @@ -5,53 +5,68 @@ import ClassNamePrefixContext from '../context/ClassNamePrefixContext'; import { StyleProps } from '../types'; import { useStyleUtilities } from './useStyleUtilities'; +export type UnsafeStylePropsResult = { + UNSAFE_className?: string; + UNSAFE_style?: CSSProperties; +}; + export type StylePropsResult = { - styleProps: HTMLAttributes; + styleProps: HTMLAttributes | UnsafeStylePropsResult; props: HTMLAttributes; }; export function useStyleProps(props: T): StylePropsResult { const classNamePrefix = useContext(ClassNamePrefixContext); - const { UNSAFE_className, UNSAFE_style, ...otherProps } = props; + const { UNSAFE_className, UNSAFE_style, ElementTag, ...otherProps } = props; const { styleUtilities, props: modifiedProps } = useStyleUtilities(otherProps, classNamePrefix); const style: CSSProperties = { ...UNSAFE_style }; - // Want to check if className prop exists, but not to define it in StyleProps type - // @ts-expect-error Property 'className' does not exist on type 'Omit'. - if (modifiedProps.className) { - warning( - false, - 'The className prop is unsafe and is unsupported in Spirit Web React. ' + - 'Please use style props with Spirit Design Tokens, or UNSAFE_className if you absolutely must do something custom. ' + - 'Note that this may break in future versions due to DOM structure changes.', - ); - - // @ts-expect-error same as above, let me live my life - delete modifiedProps.className; - } + if (typeof ElementTag !== 'function') { + // Want to check if className prop exists, but not to define it in StyleProps type + // @ts-expect-error Property 'className' does not exist on type 'Omit'. + if (modifiedProps.className) { + warning( + false, + 'The className prop is unsafe and is unsupported in Spirit Web React. ' + + 'Please use style props with Spirit Design Tokens, or UNSAFE_className if you absolutely must do something custom. ' + + 'Note that this may break in future versions due to DOM structure changes.', + ); - // Want to check if style prop exists, but not to define it in StyleProps type - // @ts-expect-error Property 'style' does not exist on type 'Omit'. - if (modifiedProps.style) { - warning( - false, - 'The style prop is unsafe and is unsupported in Spirit Web React. ' + - 'Please use style props with Spirit Design Tokens, or UNSAFE_style if you absolutely must do something custom. ' + - 'Note that this may break in future versions due to DOM structure changes.', - ); - - // @ts-expect-error same as above, let me live my life - delete modifiedProps.style; - } + // @ts-expect-error same as above, let me live my life + delete modifiedProps.className; + } - const styleProps = { - style: Object.keys(style).length > 0 ? style : undefined, - className: classNames(UNSAFE_className, ...styleUtilities) || undefined, - }; + // Want to check if style prop exists, but not to define it in StyleProps type + // @ts-expect-error Property 'style' does not exist on type 'Omit'. + if (modifiedProps.style) { + warning( + false, + 'The style prop is unsafe and is unsupported in Spirit Web React. ' + + 'Please use style props with Spirit Design Tokens, or UNSAFE_style if you absolutely must do something custom. ' + + 'Note that this may break in future versions due to DOM structure changes.', + ); + + // @ts-expect-error same as above, let me live my life + delete modifiedProps.style; + } + + const styleProps = { + style: Object.keys(style).length > 0 ? style : undefined, + className: classNames(UNSAFE_className, ...styleUtilities) || undefined, + }; + + return { + styleProps, + props: modifiedProps as HTMLAttributes, + }; + } return { - styleProps, - props: modifiedProps as HTMLAttributes, + styleProps: { + ...(UNSAFE_style !== undefined && { UNSAFE_style }), + ...(UNSAFE_className !== undefined && { UNSAFE_className }), + }, + props: { ...modifiedProps } as HTMLAttributes, }; } diff --git a/packages/web-react/src/types/shared/style.ts b/packages/web-react/src/types/shared/style.ts index 3bf1577dd0..96151d2ccc 100644 --- a/packages/web-react/src/types/shared/style.ts +++ b/packages/web-react/src/types/shared/style.ts @@ -1,4 +1,4 @@ -import { CSSProperties } from 'react'; +import { CSSProperties, ElementType } from 'react'; import { SpacingStyleProp } from '../../constants'; import { BreakpointToken, SpaceToken } from './tokens'; @@ -20,7 +20,10 @@ export interface SpacingCSSProperties extends CSSProperties { [index: `--${string}`]: string | undefined | number; } +type ElementTagType = string | ElementType; + export interface StyleProps extends SpacingProps { + ElementTag?: ElementTagType; // For backward compatibility! /** Sets the CSS [className](https://developer.mozilla.org/en-US/docs/Web/API/Element/className) for the element. Only use as a **last resort**. Use style props instead. */ UNSAFE_className?: string;