Skip to content

fix(storage): migrations incorrectly running #2130

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.

Sign up for GitHub

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

Closed
42willow wants to merge 4 commits into from
Closed

fix(storage): migrations incorrectly running #2130

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0bf5ccc
fix(storage): migrations incorrectly running
42willow Feb 14, 2026
b100c94
fix(storage): move fix to migration fn
42willow Feb 14, 2026
b84e030
tests(storage): should not run migrations on init
42willow Feb 14, 2026
7f22875
Merge branch 'main' into fix/migration-init
42willow Feb 18, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 11 additions & 7 deletions packages/storage/src/__tests__/index.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -706,16 +706,18 @@ describe('Storage Utils', () => {
expect(migrateToV3).toBeCalledWith(4);
});

it('should set the version without running migrations for empty storage items', async () => {
it('should not run migrations on initialisation', async () => {
const migrate = vi.fn((n: number) => n * 2);

const item = storage.defineItem<number>('local:key', {
init: () => 1,
let item = storage.defineItem<number>('local:key', {
version: 2,
migrations: {
2: migrate,
},
});
await item.setValue(1);
await item.migrate();

Comment on lines -709 to +720
Copy link
Copy Markdown
Member

@aklinker1 aklinker1 Feb 19, 2026

Choose a reason for hiding this comment

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

This test is still valid I think. Can you create a new test instead of editing this one?

Though I think the test name should have " with init defined".

const value = await item.getValue();
const meta = await item.getMeta();

Expand Down Expand Up @@ -789,7 +791,7 @@ describe('Storage Utils', () => {
const actualMeta = await item.getMeta();

expect(actualValue).toEqual(0);
expect(actualMeta).toEqual({});
expect(actualMeta).toEqual({ v: 3 });
Comment on lines -792 to +794
Copy link
Copy Markdown
Member

@aklinker1 aklinker1 Feb 19, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Edit: Ignore this comment, I didn't read the new issue and missed a few comments on the PR. I understand the problem better now and am considering the PR again

Details

This test is was correct - defaultValue is not the same as init:

  • Default values are returned when no value is found in storage, but it doesn't set a value in storage.
  • Items with init do set a value in storage when no value is found

That means init should set the meta, since there's now a value to control the version of. But for default values, there is no value set in storage, nothing to control the version of.

Here's an example:

// v1
const blockedUrls = defineStorageItem<string[]>("local:blockedUrls", {
  defaultValue: [],
})

// v2
const blockedUrls = defineStorageItem<{ id: string, url: string }>("local:blockedUrls", {
  defaultValue: [],
  version: 2,
  migrations: {
    2: (v1) => v1.map((url, i) => ({ id: String(i), url })),
  },
})

If a user never updates this storage item, why do we need to track that a blank array is v1 or v2? Either way, it works. But not adding meta will mean storage is less poluted and takes up less space. Hence why we don't set the version for default values.

Copy link
Copy Markdown
Member

@aklinker1 aklinker1 Feb 19, 2026

Choose a reason for hiding this comment

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

OK, with more context, I still believe that this test was valid... I'd prefer to not add a version for a null value in storage.

That said, if there's not a better place to put the new logic, we may have to accept this behavioral change.


expect(migrateToV2).not.toBeCalled();
expect(migrateToV3).not.toBeCalled();
Expand Down Expand Up @@ -899,17 +901,19 @@ describe('Storage Utils', () => {
});

it('should handle errors in migration functions', async () => {
const key = 'local:key';
await storage.setItem(key, 1);

const cause = Error('Some error');
const expectedError = new MigrationError('local:key', 2, { cause });
const item = storage.defineItem<number>('local:key', {
const expectedError = new MigrationError(key, 2, { cause });
const item = storage.defineItem<number>(key, {
version: 3,
migrations: {
2: () => {
throw cause;
},
},
});
await fakeBrowser.storage.local.set({ key: 1, key$: { v: 1 } });

await expect(item.migrate()).rejects.toThrow(expectedError);
});
Comment on lines 903 to 919
Copy link
Copy Markdown
Member

@aklinker1 aklinker1 Feb 19, 2026

Choose a reason for hiding this comment

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

Once again, I beileve the previous test is still valid. Add a new one instead.

Expand Down
11 changes: 7 additions & 4 deletions packages/storage/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -415,7 +415,13 @@ function createStorage(): WxtStorage {
driverKey,
driverMetaKey,
]);
if (value == null) return;
if (value == null) {
// Prevent running migrations if initiated at a version >= 2
if (targetVersion >= 2) {
await setMeta(driver, driverKey, { v: targetVersion });
}
return;
}
Comment thread
42willow marked this conversation as resolved.

const currentVersion = meta?.v ?? 1;
if (currentVersion > targetVersion) {
Expand Down Expand Up @@ -491,9 +497,6 @@ function createStorage(): WxtStorage {

const newValue = await opts.init();
await driver.setItem<any>(driverKey, newValue);
if (value == null && targetVersion > 1) {
await setMeta(driver, driverKey, { v: targetVersion });
}
return newValue;
});

Expand Down