This is createMigrate code allowing to async migrations.
This could potentially be patched, but this code will have more responsibilities in near future (D7668), so it's better to maintain this that way.
Depends on D7289
Details
Details
- Run tests
- Add one more async migration and check if it works
Tests could not be added directly, this function will read something from local storage (D7668) - that being said this test will fail, but code I used:
// @flow
import { creatAsyncMigrate } from './create-async-migrate.js';
describe('async redux migrations', () => {
it('should run migrations with async functions', async () => {
const persistedState = {
oldKey: 'oldKeyValue',
_persist: {
version: 1,
rehydrated: false,
},
};
const asyncMigrations = {
[1]: async state => {
return {
...state,
oldKey: 'oldKeyValue',
};
},
// needs to run
[2]: async state => {
return {
...state,
newKey: 'newKeyValue',
};
},
// needs to run
[3]: async state => {
return {
...state,
oldKey: 'oldKeyUpdated',
};
},
};
const migrate = creatAsyncMigrate(asyncMigrations, {
debug: true,
});
const currentVersion = 3;
const migratedState = await migrate(persistedState, currentVersion);
expect(migratedState).toEqual({
oldKey: 'oldKeyUpdated',
newKey: 'newKeyValue',
_persist: {
version: 1,
rehydrated: false,
},
});
});
it(`should do nothing when inboundVersion and currentVersion match`, async () => {
const persistedState = {
oldKey: 'oldKeyValue',
_persist: {
version: 3,
rehydrated: false,
},
};
const asyncMigrations = {
[1]: async state => {
return {
...state,
oldKey: 'oldKeyValue',
};
},
[2]: async state => {
return {
...state,
newKey: 'newKeyValue',
};
},
[3]: async state => {
return {
...state,
oldKey: 'oldKeyUpdated',
};
},
};
const migrate = creatAsyncMigrate(asyncMigrations, {
debug: true,
});
const currentVersion = 3;
const migratedState = await migrate(persistedState, currentVersion);
expect(migratedState).toEqual(persistedState);
});
it(`should return undefined for undefined persisted state`, async () => {
const persistedState = undefined;
const asyncMigrations = {
[1]: async state => {
return {
...state,
oldKey: 'oldKeyValue',
};
},
};
const migrate = creatAsyncMigrate(asyncMigrations, {
debug: true,
});
const currentVersion = 1;
const migratedState = await migrate(persistedState, currentVersion);
expect(migratedState).toBeUndefined();
});
});Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
| web/redux/create-async-migrate.js | ||
|---|---|---|
| 27 ↗ | (On Diff #25832) | Is this correct to skip this? I may be wrong, but some redux functions accept State | void and it might be valid for a migration to receive undefined and return something. Not a big issue though, as that shouldn't happen in our case. |
| 51 ↗ | (On Diff #25832) | In MigrationManifest we allow string as a key. Is it a good idea to sort them like that? |
| 65 ↗ | (On Diff #25832) | Can this be falsy? |
| 66 ↗ | (On Diff #25832) | What is the plan for handling exceptions here? Do we want them to be just thrown which would mean that all the migrations failed? |
Comment Actions
@tomek questions you added are valid but those are questions to the redux-persist author. I wanted to copy-paste code from source code to preserve old logic, as this returns function which will be used inside redux-persist logic. I only updated from the sync function which returns a promise - to implicitly return a promise from the async function (which will allow us to put await before any migration).
Any additional modification could cause some undefined behaviors, but if you feel strongly about investigating, I can try to improve this code in our use case.
| web/redux/create-async-migrate.js | ||
|---|---|---|
| 27 ↗ | (On Diff #25832) | It might happen in our case, so let's discuss it in place where it will be introduced (D7668) |
Comment Actions
Ah, that makes sense! But now it brings more serious issue: we shouldn't just copy and paste code from a library. Ideally, we would like to avoid modification and e.g. wrap code with our logic. If that's not possible, we can e.g. patch the library. If using this code directly is the only option, we should rewrite this code using the original one as an inspiration.
Comment Actions
Due to complexity and some legacy problems patching the library was too hard. I deeply analyzed function from redux-persist and based on this knowledge (arguments, return type, when can throw, what should do) I provided my own implementation, using existing one only as an inspiration.
| web/redux/create-async-migrate.js | ||
|---|---|---|
| 27 ↗ | (On Diff #26354) | Do we use a similar approach in other parts of the codebase? |
| web/redux/create-async-migrate.js | ||
|---|---|---|
| 27 ↗ | (On Diff #26354) | Hmm, probably not, but the previous version of the function worked like this so I think It'll be best to keep it as it was |