From 73c2b1088a1c4ab308fd6f71e5148bffc74c546b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mime=20=C4=8Cuvalo?= Date: Fri, 14 Jun 2024 11:01:50 +0100 Subject: [PATCH] image: follow-up fixes for LOD (#3934) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit couple fixes and improvements for the LOD work. - add `format=auto` for Cloudflare to send back more modern image formats - fix the broken asset logic that regressed (should not have looked at `url`) - fix stray parenthesis, omg - rm the `useValueDebounced` function in lieu of just debouncing the resolver. the problem was that the initial load in a multiplayer room has a zoom of 1 but then the real zoom comes in (via the url) and so we would double load all images 😬. this switches the debouncing to the resolving stage, not making it tied to the zoom specifically. ### Change Type - [x] `sdk` — Changes the tldraw SDK - [ ] `dotcom` — Changes the tldraw.com web app - [ ] `docs` — Changes to the documentation, examples, or templates. - [ ] `vs code` — Changes to the vscode plugin - [ ] `internal` — Does not affect user-facing stuff - [x] `bugfix` — Bug fix - [ ] `feature` — New feature - [ ] `improvement` — Improving existing features - [ ] `chore` — Updating dependencies, other boring stuff - [ ] `galaxy brain` — Architectural changes - [ ] `tests` — Changes to any test code - [ ] `tools` — Changes to infrastructure, CI, internal scripts, debugging tools, etc. - [ ] `dunno` — I don't know --- apps/dotcom/src/utils/assetHandler.test.ts | 10 +++--- apps/dotcom/src/utils/assetHandler.ts | 2 +- packages/editor/api-report.md | 3 -- packages/editor/src/index.ts | 1 - packages/state/api-report.md | 3 -- packages/state/src/lib/react/index.ts | 1 - .../state/src/lib/react/useValueDebounced.ts | 33 ------------------- packages/tldraw/api-report.md | 2 +- .../src/lib/shapes/image/ImageShapeUtil.tsx | 9 +++-- .../tldraw/src/lib/shapes/shared/useAsset.ts | 27 ++++++++------- .../src/lib/shapes/video/VideoShapeUtil.tsx | 8 ++--- 11 files changed, 30 insertions(+), 69 deletions(-) delete mode 100644 packages/state/src/lib/react/useValueDebounced.ts diff --git a/apps/dotcom/src/utils/assetHandler.test.ts b/apps/dotcom/src/utils/assetHandler.test.ts index 5889d881a..79459029d 100644 --- a/apps/dotcom/src/utils/assetHandler.test.ts +++ b/apps/dotcom/src/utils/assetHandler.test.ts @@ -128,7 +128,7 @@ describe('resolveAsset', () => { networkEffectiveType: null, }) ).toBe( - 'https://localhost:8788/cdn-cgi/image/width=50,dpr=2,fit=scale-down,quality=92/http://example.com/image.jpg' + 'https://localhost:8788/cdn-cgi/image/format=auto,width=50,dpr=2,fit=scale-down,quality=92/http://example.com/image.jpg' ) }) @@ -145,7 +145,7 @@ describe('resolveAsset', () => { networkEffectiveType: '3g', }) ).toBe( - 'https://localhost:8788/cdn-cgi/image/width=25,dpr=2,fit=scale-down,quality=92/http://example.com/image.jpg' + 'https://localhost:8788/cdn-cgi/image/format=auto,width=25,dpr=2,fit=scale-down,quality=92/http://example.com/image.jpg' ) }) @@ -162,7 +162,7 @@ describe('resolveAsset', () => { networkEffectiveType: '4g', }) ).toBe( - 'https://localhost:8788/cdn-cgi/image/width=400,dpr=1,fit=scale-down,quality=92/https://example.com/image.jpg' + 'https://localhost:8788/cdn-cgi/image/format=auto,width=400,dpr=1,fit=scale-down,quality=92/https://example.com/image.jpg' ) }) @@ -179,7 +179,7 @@ describe('resolveAsset', () => { networkEffectiveType: '2g', }) ).toBe( - 'https://localhost:8788/cdn-cgi/image/width=25,dpr=1,fit=scale-down,quality=92/https://example.com/image.jpg' + 'https://localhost:8788/cdn-cgi/image/format=auto,width=25,dpr=1,fit=scale-down,quality=92/https://example.com/image.jpg' ) }) @@ -196,7 +196,7 @@ describe('resolveAsset', () => { networkEffectiveType: '4g', }) ).toBe( - 'https://localhost:8788/cdn-cgi/image/width=25,dpr=1,fit=scale-down,quality=92/https://example.com/image.jpg' + 'https://localhost:8788/cdn-cgi/image/format=auto,width=25,dpr=1,fit=scale-down,quality=92/https://example.com/image.jpg' ) }) }) diff --git a/apps/dotcom/src/utils/assetHandler.ts b/apps/dotcom/src/utils/assetHandler.ts index dff954def..ec596c817 100644 --- a/apps/dotcom/src/utils/assetHandler.ts +++ b/apps/dotcom/src/utils/assetHandler.ts @@ -36,5 +36,5 @@ export async function resolveAsset(asset: TLAsset | null | undefined, context: A // On preview, builds the origin for the asset won't be the right one for the Cloudflare transform. const src = asset.props.src.replace(ASSET_UPLOADER_URL, ASSET_BUCKET_ORIGIN) - return `${ASSET_BUCKET_ORIGIN}/cdn-cgi/image/width=${width},dpr=${context.dpr},fit=scale-down,quality=92/${src}` + return `${ASSET_BUCKET_ORIGIN}/cdn-cgi/image/format=auto,width=${width},dpr=${context.dpr},fit=scale-down,quality=92/${src}` } diff --git a/packages/editor/api-report.md b/packages/editor/api-report.md index 7d21e6b8c..08d33ad17 100644 --- a/packages/editor/api-report.md +++ b/packages/editor/api-report.md @@ -85,7 +85,6 @@ import { useComputed } from '@tldraw/state'; import { useQuickReactor } from '@tldraw/state'; import { useReactor } from '@tldraw/state'; import { useValue } from '@tldraw/state'; -import { useValueDebounced } from '@tldraw/state'; import { VecModel } from '@tldraw/tlschema'; import { whyAmIRunning } from '@tldraw/state'; @@ -3480,8 +3479,6 @@ export function useTransform(ref: React.RefObject, x?: export { useValue } -export { useValueDebounced } - // @public (undocumented) export class Vec { constructor(x?: number, y?: number, z?: number); diff --git a/packages/editor/src/index.ts b/packages/editor/src/index.ts index 7778ef17a..8b77d470b 100644 --- a/packages/editor/src/index.ts +++ b/packages/editor/src/index.ts @@ -28,7 +28,6 @@ export { useQuickReactor, useReactor, useValue, - useValueDebounced, whyAmIRunning, type Atom, type Signal, diff --git a/packages/state/api-report.md b/packages/state/api-report.md index 87e7c4a74..3f6e11689 100644 --- a/packages/state/api-report.md +++ b/packages/state/api-report.md @@ -207,9 +207,6 @@ export function useValue(value: Signal): Value; // @public (undocumented) export function useValue(name: string, fn: () => Value, deps: unknown[]): Value; -// @public -export function useValueDebounced(name: string, fn: () => Value, deps: unknown[], ms: number): Value; - // @public export function whyAmIRunning(): void; diff --git a/packages/state/src/lib/react/index.ts b/packages/state/src/lib/react/index.ts index bf1598177..c56b51d82 100644 --- a/packages/state/src/lib/react/index.ts +++ b/packages/state/src/lib/react/index.ts @@ -5,4 +5,3 @@ export { useQuickReactor } from './useQuickReactor' export { useReactor } from './useReactor' export { useStateTracking } from './useStateTracking' export { useValue } from './useValue' -export { useValueDebounced } from './useValueDebounced' diff --git a/packages/state/src/lib/react/useValueDebounced.ts b/packages/state/src/lib/react/useValueDebounced.ts deleted file mode 100644 index db1712b11..000000000 --- a/packages/state/src/lib/react/useValueDebounced.ts +++ /dev/null @@ -1,33 +0,0 @@ -/* eslint-disable prefer-rest-params */ -import { useEffect, useState } from 'react' -import { useValue } from './useValue' - -/** - * Extracts the value from a signal and subscribes to it, debouncing the value by the given number of milliseconds. - * - * @see [[useValue]] for more information. - * - * @public - */ -export function useValueDebounced( - name: string, - fn: () => Value, - deps: unknown[], - ms: number -): Value -/** @public */ -export function useValueDebounced(): Value { - const args = [...arguments].slice(0, -1) as Parameters - const ms = arguments[arguments.length - 1] as number - const value = useValue(...args) as Value - const [debouncedValue, setDebouncedValue] = useState(value) - - useEffect(() => { - const timer = setTimeout(() => { - setDebouncedValue(value) - }, ms) - return () => clearTimeout(timer) - }, [value, ms]) - - return debouncedValue -} diff --git a/packages/tldraw/api-report.md b/packages/tldraw/api-report.md index ec70e92d8..b1a7fa08a 100644 --- a/packages/tldraw/api-report.md +++ b/packages/tldraw/api-report.md @@ -963,7 +963,7 @@ export class ImageShapeUtil extends BaseBoxShapeUtil { // (undocumented) canCrop: () => boolean; // (undocumented) - component(shape: TLImageShape): JSX_2.Element; + component(shape: TLImageShape): JSX_2.Element | null; // (undocumented) getDefaultProps(): TLImageShape['props']; // (undocumented) diff --git a/packages/tldraw/src/lib/shapes/image/ImageShapeUtil.tsx b/packages/tldraw/src/lib/shapes/image/ImageShapeUtil.tsx index 131b339cc..d9eb3a710 100644 --- a/packages/tldraw/src/lib/shapes/image/ImageShapeUtil.tsx +++ b/packages/tldraw/src/lib/shapes/image/ImageShapeUtil.tsx @@ -73,7 +73,6 @@ export class ImageShapeUtil extends BaseBoxShapeUtil { // cause visual flickering when the image is loaded. if (url && !this.isAnimated(shape)) { let cancelled = false - if (!url) return const image = Image() image.onload = () => { @@ -91,7 +90,6 @@ export class ImageShapeUtil extends BaseBoxShapeUtil { useEffect(() => { if (url && this.isAnimated(shape)) { let cancelled = false - if (!url) return const image = Image() image.onload = () => { @@ -129,7 +127,8 @@ export class ImageShapeUtil extends BaseBoxShapeUtil { const containerStyle = getCroppedContainerStyle(shape) - if (!url) { + // This is specifically `asset?.props.src` and not `url` because we're looking for broken assets. + if (!asset?.props.src) { return ( {
{asset ? null : }
- ) {'url' in shape.props && shape.props.url && ( )} @@ -153,6 +151,8 @@ export class ImageShapeUtil extends BaseBoxShapeUtil { ) } + if (!loadedSrc) return null + return ( <> {showCropPreview && ( @@ -189,7 +189,6 @@ export class ImageShapeUtil extends BaseBoxShapeUtil {
GIF
)} - ) {shape.props.url && ( )} diff --git a/packages/tldraw/src/lib/shapes/shared/useAsset.ts b/packages/tldraw/src/lib/shapes/shared/useAsset.ts index e6cf3474a..0e5724011 100644 --- a/packages/tldraw/src/lib/shapes/shared/useAsset.ts +++ b/packages/tldraw/src/lib/shapes/shared/useAsset.ts @@ -1,4 +1,4 @@ -import { TLAssetId, useEditor, useValueDebounced } from '@tldraw/editor' +import { TLAssetId, useEditor, useValue } from '@tldraw/editor' import { useEffect, useState } from 'react' /** @internal */ @@ -10,22 +10,25 @@ export function useAsset(assetId: TLAssetId | null, width: number) { const shapeScale = asset && 'w' in asset.props ? width / asset.props.w : 1 // We debounce the zoom level to reduce the number of times we fetch a new image and, // more importantly, to not cause zooming in and out to feel janky. - const debouncedScreenScale = useValueDebounced( - 'zoom level', - () => editor.getZoomLevel() * shapeScale, - [editor, shapeScale], - 500 - ) + const screenScale = useValue('zoom level', () => editor.getZoomLevel() * shapeScale, [ + editor, + shapeScale, + ]) useEffect(() => { - async function resolve() { + let isCancelled = false + const timer = editor.timers.setTimeout(async () => { const resolvedUrl = await editor.resolveAssetUrl(assetId, { - screenScale: debouncedScreenScale, + screenScale, }) - setUrl(resolvedUrl) + if (!isCancelled) setUrl(resolvedUrl) + }) + + return () => { + clearTimeout(timer) + isCancelled = true } - resolve() - }, [assetId, debouncedScreenScale, editor]) + }, [assetId, screenScale, editor]) return { asset, url } } diff --git a/packages/tldraw/src/lib/shapes/video/VideoShapeUtil.tsx b/packages/tldraw/src/lib/shapes/video/VideoShapeUtil.tsx index 1fd8d0325..712dca02e 100644 --- a/packages/tldraw/src/lib/shapes/video/VideoShapeUtil.tsx +++ b/packages/tldraw/src/lib/shapes/video/VideoShapeUtil.tsx @@ -158,7 +158,9 @@ export class VideoShapeUtil extends BaseBoxShapeUtil { >
- {url ? ( + {!asset?.props.src ? ( + + ) : url ? ( - ) : ( - - )} + ) : null}