Fold redux-persist into the codebase. #2915
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We often find us in a situation where the redux-persist API is not as well-crafted as it could be, or where the redux-persist API is lacking features, especially with regards to the internal storage implementation. redux-persist v5 offers new features, but they change the internal storage implementation in a way that would leave us off worse. In particular, the built-in migration system in redux-persist v5 does not seem future-proof. In general, the whole library seems to lack rigor and thought-through-ness. Therefore, fold redux-persist v4.10.2 into the codebase. (Prior to this commit, we were using v4.10.0). This will allow us to quickly iterate on enhancing the API, and to rework the internal storage implementation. This commit clones the https://github.com/rt2zz/redux-persist repository at the v4.10.2 tag. All the relevant source code resides in redux-persist/src, and all the relevant flow type definitions in redux-persist/src/type-definitions/index.js.flow. All other files and directories are discarded. The relevant dependencies of this pruned redux-persist repo are copied to zulip-mobile's package.json. The dependency on npm's `redux-persist` is removed, and all imports from that package are rerouted. All source files in redux-persist are unchanged, except the `import isPlainObject from 'lodash/isPlainObject'` -- it gets changed to `import isPlainObject from 'lodash.isplainobject'` along with the respective dependency. The redux-persist codebase does not adhere the eslint and prettier style guides, so tests will fail. The following commit will fix this.
The following commit is due to follow. |
This is mostly syntactic enhancements. The following semantic changes are applied: * replace `console.*` with `logErrorRemotely()`
|
Updated to format redux-persist with prettier and eslint. Ready for review. |
chrisbobbe
pushed a commit
to chrisbobbe/zulip-mobile
that referenced
this pull request
Apr 17, 2020
…d of string. This commit should be read for its contrast with its counterpart, with the same title, at zulip#3952. I've tried to make this PR minimally different from that one, so a `git diff` or `git range-diff` might be informative. Here, we use the "inside-out" approach described there: > (i.e., locating the value by its being `instanceof ZulipVersion`), > which doesn't depend on knowing the value's location in the Redux > state As expected, it requires writing a (straightforward) migration. Unfortunately, I haven't gotten far enough to actually see this working, but I expect that our code in a working version won't need to be drastically different, so I'm submitting this for comparison anyhow. There appear to be some inaccuracies in the Flow libdef I cobbled together from the TypeScript with `flowgen` and `flow-typed create-stub`, but these shouldn't be difficult to fix. The current blocker, which blocks any use of ImmutableJS with Redux (an integration this PR depends on, as I noted at the bottom of zulip#3951 (comment)), is that we're stuck with an old version of `redux-persist`. Not until rt2zz/redux-persist@a94f29139, released in v6.0.0 (we're on ^4.10.2), do we get the ability to supply our own serialize/deserialize functions in the config passed to `persistStore`. The only other customization for replace/revive transforms that `redux-persist` allows is via a Transform created by `createTransform`, as we use in zulip#3952. But its contract requires that you supply functions that translate between serializable and non-serializable objects, without allowing us to take responsibility for the actual serialization. It would be a silly exercise, but it might be possible, to make it work with `createTransform` with an extra round-trip into JSAN [sic]. JSAN is what `remotedev-serialize` serializes into, and it claims to be more flexible than JSON. I believe zulip#2915 is our latest written knowledge about the drawbacks of `redux-persist` and why it ceases to be useful for us at version 5 and above, and it looks like we have a plan to vendor it.
chrisbobbe
pushed a commit
to chrisbobbe/zulip-mobile
that referenced
this pull request
Apr 21, 2020
In zulip#2762 (never merged), we tried to use v5 of redux-persist and found that it had breaking changes that unacceptably reduced our flexibility with migrations, so we decided never to use it, and instead to vendor it and make our own changes as needed. More background on the limitations of v5 is at https://chat.zulip.org/#narrow/stream/243-mobile-team/subject/redux-persist.20v5/near/624465. Its new way of doing migrations means you can't write a migration that is aware of the entire state tree, unless you limit yourself to storing the entire tree in a single key-value entry. We need to be able to split it up for performance (avoiding a rewrite to disk of everything each time a small change is made) and because each key-value pair is has a hard limit of 2MB, after which the data is just dropped. Even with compression, we're within a factor of 4 of this limit, which is unacceptable. More generally, the redux-persist maintainers seem not to have deep familiarity with apps that depend on correct, effective migrations to persist data across updates. Our immediate need is to incorporate the changes from rt2zz/redux-persist@a94f29139 and rt2zz/redux-persist@13a0b600b (not released until 6.0.0) so that we can pass custom serialize/deserialize functions for replace/revive logic so we can store the non-serializable ZulipVersion in Redux. PR zulip#2915 was opened a while ago to vendor redux-persist, given the above, but it grew stale and gathered merge conflicts. First, ensure that the version of redux-persist in node_modules is 4.10.2, which is what we'd been using. Then, copy the contents of `node_modules/redux-persist/src` to `src/third/redux-persist` (this contains all the needed source code) and copy `node_modules/redux-persist/es/index.js.flow` there as well (it has the needed type definitions). Then, address redux-persist's two dependencies that weren't listed in our own package.json: `json-stringify-safe` and `lodash`. For the former, just add it to package.json. For the latter, note that the only bit of lodash that it uses is `isPlainObject`, so add `lodash.isplainobject` to the package.json and make the only change to our vendored copy's source code: In `src/third/redux-persist/utils/isStatePlainEnough.js: ```diff - import isPlainObject from 'lodash/isPlainObject' + import isPlainObject from 'lodash.isplainobject' ``` Then run `yarn`. Rather than fixing all the issues ESLint and Prettier flagged, tell those tools to ignore them -- we didn't introduce them, and we'd rather not accidentally introduce bugs by even these small changes. Some of the output indicated potential code-quality issues (see note at `no-use-before-define` in .eslintrc.yaml), some, just differing style choices (e.g., no semicolons), and others, just the fact that redux-persist wasn't using the very latest JavaScript features.
chrisbobbe
pushed a commit
to chrisbobbe/zulip-mobile
that referenced
this pull request
Apr 29, 2020
In zulip#2762 (never merged), we tried to use v5 of redux-persist and found that it had breaking changes that unacceptably reduced our flexibility with migrations, so we decided never to use it, and instead to vendor it and make our own changes as needed. More background on the limitations of v5 is at https://chat.zulip.org/#narrow/stream/243-mobile-team/subject/redux-persist.20v5/near/624465. Its new way of doing migrations means you can't write a migration that is aware of the entire state tree, unless you limit yourself to storing the entire tree in a single key-value entry. We need to be able to split it up for performance (avoiding a rewrite to disk of everything each time a small change is made) and because each key-value pair is has a hard limit of 2MB, after which the data is just dropped. Even after compression, our entire state tree would be within a factor of 4 of this limit, which is unacceptable. More generally, the redux-persist maintainers seem not to have deep familiarity with apps that depend on correct, effective migrations to persist data across updates. Our immediate need is to incorporate the changes from rt2zz/redux-persist@a94f29139 and rt2zz/redux-persist@13a0b600b (not released until 6.0.0) so that we can pass custom serialize/deserialize functions for replace/revive logic so we can store non-serializable data (such as a ZulipVersion) in Redux. PR zulip#2915 was opened a while ago to vendor redux-persist, but it grew stale and gathered merge conflicts. First, ensure that the version of redux-persist in node_modules is 4.10.2, which is what we'd been using. Then, copy the contents of `node_modules/redux-persist/src` to `src/third/redux-persist` (this contains all the needed source code) and copy `node_modules/redux-persist/es/index.js.flow` there too; it has the needed type definitions. Then, address redux-persist's two dependencies that weren't listed in our own package.json: - `json-stringify-safe`: Add to package.json. - `lodash`: The only bit of lodash that it uses is `isPlainObject`, so add `lodash.isplainobject` to the package.json and make this change, the only change to our vendored copy's source code: In `src/third/redux-persist/utils/isStatePlainEnough.js: ```diff - import isPlainObject from 'lodash/isPlainObject' + import isPlainObject from 'lodash.isplainobject' ``` Then run `yarn`. Rather than fixing all the issues ESLint and Prettier flagged, tell those tools to ignore them -- we didn't introduce them, and we'd rather not accidentally introduce bugs by even these small changes. Some of the output indicated potential code-quality issues (see note at `no-use-before-define` in .eslintrc.yaml), some, just differing style choices (e.g., no semicolons), and others, just the fact that redux-persist wasn't using the very latest JavaScript features.
chrisbobbe
pushed a commit
to chrisbobbe/zulip-mobile
that referenced
this pull request
May 1, 2020
In zulip#2762 (never merged), we tried to use v5 of redux-persist and found that it had breaking changes that unacceptably reduced our flexibility with migrations, so we decided never to use it, and instead to vendor it and make our own changes as needed. More background on the limitations of v5 is at https://chat.zulip.org/#narrow/stream/243-mobile-team/subject/redux-persist.20v5/near/624465. Its new way of doing migrations means you can't write a migration that is aware of the entire state tree, unless you limit yourself to storing the entire tree in a single key-value entry. We need to be able to split it up for performance (avoiding a rewrite to disk of everything each time a small change is made) and because each key-value pair is has a hard limit of 2MB, after which the data is just dropped. Even after compression, our entire state tree would be within a factor of 4 of this limit, which is unacceptable. More generally, the redux-persist maintainers seem not to have deep familiarity with apps that depend on correct, effective migrations to persist data across updates. Our immediate need is to incorporate the changes from rt2zz/redux-persist@a94f29139 and rt2zz/redux-persist@13a0b600b (not released until 6.0.0) so that we can pass custom serialize/deserialize functions for replace/revive logic so we can store non-serializable data (such as a ZulipVersion) in Redux. PR zulip#2915 was opened a while ago to vendor redux-persist, but it grew stale and gathered merge conflicts. First, ensure that the version of redux-persist in node_modules is 4.10.2, which is what we'd been using. Then, copy the contents of `node_modules/redux-persist/src` to `src/third/redux-persist` (this contains all the needed source code) and copy `node_modules/redux-persist/es/index.js.flow` there too; it has the needed type definitions. Then, address redux-persist's two dependencies that weren't listed in our own package.json: - `json-stringify-safe`: Add to package.json. - `lodash`: The only bit of lodash that it uses is `isPlainObject`, so add `lodash.isplainobject` to the package.json and make this change, the only change to our vendored copy's source code: In `src/third/redux-persist/utils/isStatePlainEnough.js: ```diff - import isPlainObject from 'lodash/isPlainObject' + import isPlainObject from 'lodash.isplainobject' ``` Then run `yarn`. Rather than fixing all the issues ESLint and Prettier flagged, tell those tools to ignore them -- we didn't introduce them, and we'd rather not accidentally introduce bugs by even these small changes. Some of the output indicated potential code-quality issues (see note at `no-use-before-define` in .eslintrc.yaml), some, just differing style choices (e.g., no semicolons), and others, just the fact that redux-persist wasn't using the very latest JavaScript features.
chrisbobbe
pushed a commit
to chrisbobbe/zulip-mobile
that referenced
this pull request
May 1, 2020
In zulip#2762 (never merged), we tried to use v5 of redux-persist and found that it had breaking changes that unacceptably reduced our flexibility with migrations, so we decided never to use it, and instead to vendor it and make our own changes as needed. More background on the limitations of v5 is at https://chat.zulip.org/#narrow/stream/243-mobile-team/subject/redux-persist.20v5/near/624465. Its new way of doing migrations means you can't write a migration that is aware of the entire state tree, unless you limit yourself to storing the entire tree in a single key-value entry. We need to be able to split it up for performance (avoiding a rewrite to disk of everything each time a small change is made) and because each key-value pair is has a hard limit of 2MB, after which the data is just dropped. Even after compression, our entire state tree would be within a factor of 4 of this limit, which is unacceptable. More generally, the redux-persist maintainers seem not to have deep familiarity with apps that depend on correct, effective migrations to persist data across updates. Our immediate need is to incorporate the changes from rt2zz/redux-persist@a94f29139 and rt2zz/redux-persist@13a0b600b (not released until 6.0.0) so that we can pass custom serialize/deserialize functions for replace/revive logic so we can store non-serializable data (such as a ZulipVersion) in Redux. PR zulip#2915 was opened a while ago to vendor redux-persist, but it grew stale and gathered merge conflicts. First, ensure that the version of redux-persist in node_modules is 4.10.2, which is what we'd been using. Then, copy the contents of `node_modules/redux-persist/src` to `src/third/redux-persist` (this contains all the needed source code) and copy `node_modules/redux-persist/es/index.js.flow` there too; it has the needed type definitions. Then, address redux-persist's two dependencies that weren't listed in our own package.json: - `json-stringify-safe`: Add to package.json. - `lodash`: The only bit of lodash that it uses is `isPlainObject`, so add `lodash.isplainobject` to the package.json and make this change, the only change to our vendored copy's source code: In `src/third/redux-persist/utils/isStatePlainEnough.js: ```diff - import isPlainObject from 'lodash/isPlainObject' + import isPlainObject from 'lodash.isplainobject' ``` Then run `yarn`. Rather than fixing all the issues ESLint and Prettier flagged, tell those tools to ignore them -- we didn't introduce them, and we'd rather not accidentally introduce bugs by even these small changes. Some of the output indicated potential code-quality issues (see note at `no-use-before-define` in .eslintrc.yaml), some, just differing style choices (e.g., no semicolons), and others, just the fact that redux-persist wasn't using the very latest JavaScript features.
chrisbobbe
pushed a commit
to chrisbobbe/zulip-mobile
that referenced
this pull request
May 5, 2020
In zulip#2762 (never merged), we tried to use v5 of redux-persist and found that it had breaking changes that unacceptably reduced our flexibility with migrations, so we decided never to use it, and instead to vendor it and make our own changes as needed. More background on the limitations of v5 is at https://chat.zulip.org/#narrow/stream/243-mobile-team/subject/redux-persist.20v5/near/624465. Its new way of doing migrations means you can't write a migration that is aware of the entire state tree, unless you limit yourself to storing the entire tree in a single key-value entry. We need to be able to split it up for performance (avoiding a rewrite to disk of everything each time a small change is made) and because each key-value pair is has a hard limit of 2MB, after which the data is just dropped. Even after compression, our entire state tree would be within a factor of 4 of this limit, which is unacceptable. More generally, the redux-persist maintainers seem not to have deep familiarity with apps that depend on correct, effective migrations to persist data across updates. Our immediate need is to incorporate the changes from rt2zz/redux-persist@a94f29139 and rt2zz/redux-persist@13a0b600b (not released until 6.0.0) so that we can pass custom serialize/deserialize functions for replace/revive logic so we can store non-serializable data (such as a ZulipVersion) in Redux. PR zulip#2915 was opened a while ago to vendor redux-persist, but it grew stale and gathered merge conflicts. First, ensure that the version of redux-persist in node_modules is 4.10.2, which is what we'd been using. Then, copy the contents of `node_modules/redux-persist/src` to `src/third/redux-persist` (this contains all the needed source code) and copy `node_modules/redux-persist/es/index.js.flow` there too; it has the needed type definitions. Then, address redux-persist's two dependencies that weren't listed in our own package.json: - `json-stringify-safe`: Add to package.json. - `lodash`: The only bit of lodash that it uses is `isPlainObject`, so add `lodash.isplainobject` to the package.json and make this change, the only change to our vendored copy's source code: In `src/third/redux-persist/utils/isStatePlainEnough.js: ```diff - import isPlainObject from 'lodash/isPlainObject' + import isPlainObject from 'lodash.isplainobject' ``` Then run `yarn`. Rather than fixing all the issues ESLint and Prettier flagged, tell those tools to ignore them -- we didn't introduce them, and we'd rather not accidentally introduce bugs by even these small changes. Some of the output indicated potential code-quality issues (see note at `no-use-before-define` in .eslintrc.yaml), some, just differing style choices (e.g., no semicolons), and others, just the fact that redux-persist wasn't using the very latest JavaScript features.
|
We've now finally merged in a version of this change! Namely e5409c5 / #4047. Thanks @roberthoenig for your work on investigating this and figuring out what we wanted to do. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We often find us in a situation where the redux-persist API is not as
well-crafted as it could be, or where the redux-persist API is lacking
features, especially with regards to the internal storage
implementation. redux-persist v5 offers new features, but they change
the internal storage implementation in a way that would leave us off
worse. In particular, the built-in migration system in redux-persist
v5 does not seem future-proof. In general, the whole library seems to
lack rigor and thought-through-ness.
Therefore, fold redux-persist v4.10.2 into the codebase. (Prior to
this commit, we were using v4.10.0). This will allow us to quickly
iterate on enhancing the API, and to rework the internal storage
implementation.
This commit clones the https://github.com/rt2zz/redux-persist
repository at the v4.10.2 tag. All the relevant source code resides in
redux-persist/src, and all the relevant flow type definitions in
redux-persist/src/type-definitions/index.js.flow. All other files and
directories are discarded. The relevant dependencies of this pruned
redux-persist repo are copied to zulip-mobile's package.json. The
dependency on npm's
redux-persistis removed, and all imports fromthat package are rerouted. All source files in redux-persist are
unchanged, except the
import isPlainObject from 'lodash/isPlainObject'-- it gets changed toimport isPlainObject from 'lodash.isplainobject'along with the respective dependency.The redux-persist codebase does not adhere the eslint and prettier
style guides, so tests will fail. The following commit will fix this.