Skip to content

fix: fix typescript error on defineItem fallback #1601

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

Merged
merged 2 commits into from
Apr 24, 2025
Merged

Conversation

iyume
Copy link
Contributor

@iyume iyume commented Apr 22, 2025

Overview

Currently, the second signature of WxtStorage.defineItem method is incorrect that it doesn't respect the fallback value.

defineItem<TValue, TMetadata extends Record<string, unknown> = {}>(
key: StorageItemKey,
): WxtStorageItem<TValue | null, TMetadata>;
defineItem<TValue, TMetadata extends Record<string, unknown> = {}>(
key: StorageItemKey,
options: WxtStorageItemOptions<TValue>,
): WxtStorageItem<TValue, TMetadata>;

The following examples show the bug clearly.

export const storageTodoItems = storage.defineItem<StorageTodoItem[]>(
  'local:StorageTodoItems',
  { fallback: [] },
)  // Type is StorageTodoItem[]

export const storageTodoItems = storage.defineItem<StorageTodoItem[]>(
  'local:StorageTodoItems',
)  // Type is StorageTodoItem[] | null

export const storageTodoItems = storage.defineItem<StorageTodoItem[]>(
  'local:StorageTodoItems', {}
)  // Type is StorageTodoItem[]. Wrong!!!

Manual Testing

export const storageTodoItems = storage.defineItem<StorageTodoItem[]>(
  'local:StorageTodoItems',
)  // Type is StorageTodoItem[] | null

export const storageTodoItems = storage.defineItem<StorageTodoItem[]>(
  'local:StorageTodoItems',
  {},
)  // Type is StorageTodoItem[] | null

export const storageTodoItems = storage.defineItem<StorageTodoItem[]>(
  'local:StorageTodoItems',
  { fallback: [] },
)  // Type is StorageTodoItem[]

Related Issue

@iyume iyume requested review from aklinker1 and Timeraa as code owners April 22, 2025 05:54
Copy link

netlify bot commented Apr 22, 2025

Deploy Preview for creative-fairy-df92c4 ready!

Name Link
🔨 Latest commit f06bbba
🔍 Latest deploy log https://app.netlify.com/sites/creative-fairy-df92c4/deploys/6809d8c101747e000850150d
😎 Deploy Preview https://deploy-preview-1601--creative-fairy-df92c4.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Collaborator

@aklinker1 aklinker1 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, looks good!

Could you add type tests to packages/storage/src/__tests__/index.test.ts instead of just listing them in the PR description? That way we can ensure they don't break in the future.

Something like this should be good enough:

it("should consider the fallback when typing the value", () => {
  expectTypeOf(...).toBe<...>()
});

Don't worry about running vitest's type-test mode, just run pnpm check to run the typescript compiler.

@iyume
Copy link
Contributor Author

iyume commented Apr 24, 2025

@aklinker1 Thanks for the review! Tests are added for this case and passing with pnpm check.

Copy link

pkg-pr-new bot commented Apr 24, 2025

Open in StackBlitz

@wxt-dev/analytics

npm i https://pkg.pr.new/@wxt-dev/analytics@1601

@wxt-dev/auto-icons

npm i https://pkg.pr.new/@wxt-dev/auto-icons@1601

@wxt-dev/browser

npm i https://pkg.pr.new/@wxt-dev/browser@1601

@wxt-dev/i18n

npm i https://pkg.pr.new/@wxt-dev/i18n@1601

@wxt-dev/module-react

npm i https://pkg.pr.new/@wxt-dev/module-react@1601

@wxt-dev/module-solid

npm i https://pkg.pr.new/@wxt-dev/module-solid@1601

@wxt-dev/module-svelte

npm i https://pkg.pr.new/@wxt-dev/module-svelte@1601

@wxt-dev/module-vue

npm i https://pkg.pr.new/@wxt-dev/module-vue@1601

@wxt-dev/storage

npm i https://pkg.pr.new/@wxt-dev/storage@1601

@wxt-dev/unocss

npm i https://pkg.pr.new/@wxt-dev/unocss@1601

@wxt-dev/webextension-polyfill

npm i https://pkg.pr.new/@wxt-dev/webextension-polyfill@1601

wxt

npm i https://pkg.pr.new/wxt@1601

commit: f06bbba

@aklinker1 aklinker1 merged commit 8e96bfe into wxt-dev:main Apr 24, 2025
16 checks passed
Copy link
Contributor

Thanks for helping make WXT better!

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.

2 participants