-
Notifications
You must be signed in to change notification settings - Fork 209
chore: Detect and report missing css styles #3591
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 all commits
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 |
|---|---|---|
|
|
@@ -23,7 +23,7 @@ class CustomPropertyPageObject extends BasePageObject { | |
| for (const [prop, value] of (element as any).computedStyleMap()) { | ||
| // Custom Property | ||
| if (prop.startsWith('--')) { | ||
| const valueWithoutPostfix = prop.substring(2, prop.length - 7); | ||
| const valueWithoutPostfix = prop.replace(/^--/, '').replace(/-[\d\w]+$/, ''); | ||
|
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. This code assumed the hash is always 7 characters, which is not true |
||
| result[valueWithoutPostfix] = value[0][0]; | ||
| } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,3 +9,8 @@ | |
| */ | ||
| @use 'awsui:globals'; | ||
| @use '../styles/global.scss'; | ||
| @use '../generated/custom-css-properties' as custom-styles; | ||
|
|
||
| :root { | ||
| --awsui-version-info-#{custom-styles.$awsui-commit-hash}: true; | ||
|
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. Core of this check. We check presence of this CSS var in Javascript. If not found – the styles are missing
Member
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. so if an app doesn't bundle and server styles but another app on the same page has a similar git commit. it wouldn't be detected. but I guess this is acceptable, and better than what we have now.
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. Yes, there is a chance of missing some cases, if they happen to have the same commit |
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| // Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
| import React from 'react'; | ||
| import { render, waitFor } from '@testing-library/react'; | ||
|
|
||
| import { useMissingStylesCheck } from '../../../../../lib/components/internal/hooks/use-base-component/styles-check'; | ||
|
|
||
| function Test() { | ||
| useMissingStylesCheck(); | ||
| return null; | ||
| } | ||
|
|
||
| let consoleMock: jest.SpyInstance; | ||
| beforeEach(() => { | ||
| globalThis.requestIdleCallback = cb => setTimeout(cb, 0); | ||
| consoleMock = jest.spyOn(console, 'error').mockImplementation(() => {}); | ||
| }); | ||
| afterEach(() => { | ||
| jest.restoreAllMocks(); | ||
| }); | ||
|
|
||
| test('emits style check error only once', async () => { | ||
| const { rerender } = render(<Test key={1} />); | ||
|
|
||
| await waitFor( | ||
| () => { | ||
| expect(consoleMock).toHaveBeenCalledTimes(1); | ||
| expect(consoleMock).toHaveBeenCalledWith(expect.stringContaining('Missing AWS-UI CSS')); | ||
| }, | ||
| { timeout: 2000 } | ||
| ); | ||
|
|
||
| consoleMock.mockClear(); | ||
| rerender(<Test key={2} />); | ||
|
|
||
| await new Promise(resolve => setTimeout(resolve, 1100)); | ||
| expect(consoleMock).toHaveBeenCalledTimes(0); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,111 @@ | ||
| // Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| import { GIT_SHA } from '../../../../../lib/components/internal/environment'; | ||
| import { checkMissingStyles } from '../../../../../lib/components/internal/hooks/use-base-component/styles-check'; | ||
| import { metrics } from '../../../../../lib/components/internal/metrics'; | ||
| import { idleWithDelay } from '../styles-check'; | ||
|
|
||
| jest.mock('../../../../../lib/components/internal/environment', () => ({ | ||
| ...jest.requireActual('../../../../../lib/components/internal/environment'), | ||
| PACKAGE_VERSION: '3.0.0 (abc)', | ||
| GIT_SHA: 'abc', | ||
| })); | ||
|
|
||
| afterEach(() => { | ||
| jest.resetAllMocks(); | ||
| }); | ||
|
|
||
| describe('checkMissingStyles', () => { | ||
| let consoleWarnSpy: jest.SpyInstance; | ||
| let sendPanoramaMetricSpy: jest.SpyInstance; | ||
| const style = document.createElement('style'); | ||
| document.body.append(style); | ||
|
|
||
| beforeEach(() => { | ||
| style.textContent = ``; | ||
| consoleWarnSpy = jest.spyOn(console, 'error').mockImplementation(() => {}); | ||
| sendPanoramaMetricSpy = jest.spyOn(metrics, 'sendOpsMetricObject').mockImplementation(() => {}); | ||
| }); | ||
|
|
||
| test('should pass the check if styles found', () => { | ||
| // using :root does not work in JSDOM: https://github.com/jsdom/jsdom/issues/3563 | ||
| style.textContent = ` | ||
| body { | ||
| --awsui-version-info-${GIT_SHA}: true; | ||
| } | ||
| `; | ||
| checkMissingStyles(); | ||
| expect(consoleWarnSpy).not.toHaveBeenCalled(); | ||
| expect(sendPanoramaMetricSpy).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| test('should detect missing styles', () => { | ||
| checkMissingStyles(); | ||
| expect(consoleWarnSpy).toHaveBeenCalledWith( | ||
| 'Missing AWS-UI CSS for theme "default", version "3.0.0 (abc)", and git sha "abc".' | ||
| ); | ||
| expect(sendPanoramaMetricSpy).toHaveBeenCalledWith('awsui-missing-css-asset', {}); | ||
| }); | ||
|
|
||
| test('should report missing styles if a different version found', () => { | ||
| style.textContent = ` | ||
| body { | ||
| --awsui-version-info-c4d5e6: true; | ||
| } | ||
| `; | ||
| checkMissingStyles(); | ||
| expect(consoleWarnSpy).toHaveBeenCalledWith( | ||
| 'Missing AWS-UI CSS for theme "default", version "3.0.0 (abc)", and git sha "abc".' | ||
| ); | ||
| expect(sendPanoramaMetricSpy).toHaveBeenCalledWith('awsui-missing-css-asset', {}); | ||
| }); | ||
| }); | ||
|
|
||
| describe('idleWithDelay', () => { | ||
| beforeEach(() => { | ||
| jest.useFakeTimers(); | ||
| // simulate requestIdleCallback for JSDOM | ||
| globalThis.requestIdleCallback = cb => setTimeout(cb, 0); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| jest.useRealTimers(); | ||
| // @ts-expect-error reset to initial state | ||
| globalThis.requestIdleCallback = undefined; | ||
| }); | ||
|
|
||
| test('does nothing if requestIdleCallback not supported', () => { | ||
| // @ts-expect-error simulate missing API | ||
| globalThis.requestIdleCallback = undefined; | ||
| const cb = jest.fn(); | ||
| expect(requestIdleCallback).toBe(undefined); | ||
| idleWithDelay(cb); | ||
| jest.runAllTimers(); | ||
| expect(cb).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| test('runs callback after a delay', () => { | ||
| const cb = jest.fn(); | ||
| idleWithDelay(cb); | ||
| jest.runAllTimers(); | ||
| expect(cb).toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| test('delay can be aborted before setTimeout phase', () => { | ||
| const cb = jest.fn(); | ||
| const abort = idleWithDelay(cb); | ||
| abort!(); | ||
| jest.runAllTimers(); | ||
| expect(cb).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| test('delay can be aborted before requestIdleCallback phase', () => { | ||
| const cb = jest.fn(); | ||
| const abort = idleWithDelay(cb); | ||
| jest.runOnlyPendingTimers(); // flush setTimeout | ||
| abort!(); | ||
| jest.runOnlyPendingTimers(); // flush following requestIdleCallback | ||
| expect(cb).not.toHaveBeenCalled(); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,52 @@ | ||
| // Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
| import { useEffect } from 'react'; | ||
|
|
||
| import { GIT_SHA, PACKAGE_VERSION, THEME } from '../../environment'; | ||
| import { metrics } from '../../metrics'; | ||
|
|
||
| export function checkMissingStyles() { | ||
| const result = getComputedStyle(document.body).getPropertyValue(`--awsui-version-info-${GIT_SHA}`); | ||
| if (!result) { | ||
| console.error(`Missing AWS-UI CSS for theme "${THEME}", version "${PACKAGE_VERSION}", and git sha "${GIT_SHA}".`); | ||
| metrics.sendOpsMetricObject('awsui-missing-css-asset', {}); | ||
| } | ||
| } | ||
|
|
||
| export function idleWithDelay(cb: () => void) { | ||
|
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. Exported |
||
| // if idle callbacks not supported, we simply do not collect the metric | ||
| if (typeof requestIdleCallback !== 'function') { | ||
| return; | ||
| } | ||
| let aborted = false; | ||
|
|
||
| setTimeout(() => { | ||
| if (aborted) { | ||
| return; | ||
| } | ||
| requestIdleCallback(() => { | ||
| if (aborted) { | ||
| return; | ||
| } | ||
| cb(); | ||
| }); | ||
| }, 1000); | ||
|
|
||
| return () => { | ||
| aborted = true; | ||
| }; | ||
| } | ||
|
|
||
| let checked = false; | ||
| const checkMissingStylesOnce = () => { | ||
| if (!checked) { | ||
| checkMissingStyles(); | ||
| checked = true; | ||
| } | ||
| }; | ||
|
|
||
| export function useMissingStylesCheck() { | ||
| useEffect(() => { | ||
| return idleWithDelay(() => checkMissingStylesOnce()); | ||
| }, []); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,6 @@ | |
| // SPDX-License-Identifier: Apache-2.0 | ||
| import { Metrics } from '@cloudscape-design/component-toolkit/internal'; | ||
|
|
||
| import { PACKAGE_SOURCE, PACKAGE_VERSION } from './environment'; | ||
| import { PACKAGE_SOURCE, PACKAGE_VERSION, THEME } from './environment'; | ||
|
|
||
| export const metrics = new Metrics(PACKAGE_SOURCE, PACKAGE_VERSION); | ||
| export const metrics = new Metrics({ packageSource: PACKAGE_SOURCE, packageVersion: PACKAGE_VERSION, theme: THEME }); | ||
|
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. Addressing this follow-up cloudscape-design/component-toolkit#136 (comment) Yes I know this
Member
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. can you explain a bit more?
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. Before it was like this const metrics = new Metrics(PACKAGE_SOURCE, PACKAGE_VERSION);
metrics.initMetrics(theme);This is not safe, because it Multiple packages may call the same toolkit instance and the same So, now we finally got the theme properly scoped to each bundle |
||
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.
workspace.generatedPathcontains other non-generated files: https://github.com/cloudscape-design/components/tree/main/src/internal/generatedIdeally we should clean up them all (or move to a different path), but not in this PR