Skip to content

fix(image-imports): get cache - #59

Merged
FredKSchott merged 1 commit into
cloudflare:mainfrom
hyoban:2-25-image-size-cache
Feb 27, 2026
Merged

FredKSchott merged 1 commit into
cloudflare:mainfrom
hyoban:2-25-image-size-cache

Conversation

@hyoban

@hyoban hyoban commented Feb 25, 2026

Copy link
Copy Markdown
Contributor

Currently, this is pointing to the wrong value.

@southpolesteve

Copy link
Copy Markdown
Collaborator

/bigbonk review

@ask-bonk ask-bonk Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Clean fix. The load hook in Vite/Rolldown doesn't bind this to the plugin object — it binds to the Rolldown plugin context instead, so (this as any)._dimCache was always undefined. Hoisting the Map to a closure variable is the correct approach.

One minor note: now that _dimCache is no longer accessed via this, the property assignment on the plugin object (line 2577: _dimCache: imageImportDimCache) and the type assertion at line 2670 (as Plugin & { _dimCache: ... }) are dead code. Consider removing both to keep things tidy — the property only existed to support the old (this as any)._dimCache pattern.


// Cache of image dimensions to avoid re-reading files
_dimCache: new Map<string, { width: number; height: number }>(),
_dimCache: imageImportDimCache,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit: This property is now dead code — nothing reads _dimCache from the plugin object anymore. Consider removing it (and the corresponding & { _dimCache: ... } type assertion at line 2670) to keep the plugin definition clean.

@ask-bonk

ask-bonk Bot commented Feb 25, 2026

Copy link
Copy Markdown
Contributor

Review submitted. Here's a summary:

Approved — This is a correct fix. The load hook in Vite/Rolldown binds this to the Rolldown plugin context, not the plugin object itself, so (this as any)._dimCache was resolving to undefined and the cache was never working. Hoisting the Map to a closure variable (imageImportDimCache) is the right solution.

Left one minor nit: the _dimCache property on the plugin object and its type assertion are now dead code and could be cleaned up.

github run

@pkg-pr-new

pkg-pr-new Bot commented Feb 27, 2026

Copy link
Copy Markdown

Open in StackBlitz

npm i https://pkg.pr.new/vinext@59

commit: 75c00ac

@FredKSchott

Copy link
Copy Markdown
Collaborator

LGTM, thanks @hyoban !

@FredKSchott
FredKSchott enabled auto-merge (squash) February 27, 2026 05:52
@FredKSchott
FredKSchott merged commit 2061ff6 into cloudflare:main Feb 27, 2026
13 of 20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants