This differential patches localforage to support transactional multiple item retrieval/persistence. The synchronizationKey and synchronizationValue arguments are used to handle race conditions. setMultipleItems will only succeed if current value under synchronizationKey in localforage is equal to expectedSynchronizationValue or forceWrite is set to true. This lets us select on thread to always win race condition.
Details
Details
tested with next diff
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
Comment Actions
If the patch file appears cumbersome to review I will create fork of localforage with PR containing those changes.
Comment Actions
Even if we don't use localforage in our code outside of web, this patch probably affects some transitive dependencies for other projects. It looks like it's backwards-compatible, so it should be safe. But we may face some trouble at a later point if a transitive dependency upgrades its version, depending on which package Yarn hoists to the workspace root, and how patch-package handles it when multiple versions of a package are present (does it patch all of them, or only the one at the workspace root?)
| web/flow-typed/npm/localforage_v1.5.x.js | ||
|---|---|---|
| 79–82 ↗ | (On Diff #42733) | I think this will be better - wondering what you think |
Comment Actions
Running yarn why localforage gives the following result:
yarn why v1.22.19 [1/4] 🤔 Why do we have the module "localforage"...? [2/4] 🚚 Initialising dependency graph... [3/4] 🔍 Finding dependency... [4/4] 🚡 Calculating file sizes... => Found "localforage@1.10.0" info Reasons this module exists - "_project_#web" depends on it - Hoisted from "_project_#web#localforage" - Hoisted from "_project_#native#@redux-devtools#cli#@redux-devtools#app#localforage" info Disk size without dependencies: "620KB" info Disk size with unique dependencies: "700KB" info Disk size with transitive dependencies: "736KB" info Number of shared dependencies: 1 ✨ Done in 0.48s.
It means that localforage is a direct dependency of web project and transitive dependency of redux-devtools. Therefore the risks of fork vs patch outline as follow:
- patch: If we update redux-devtools it may happen that it will depend on different version of localforage and yarn will pick the wrong one to patch.
- fork: The risk of fork is that we might miss some update of this library in future and it also requires effort from the team to create and maintain.
Both risks as pretty low though since new localforage version hasn't been released in 3 years and this repo has more than 1k of fork (which suggests authors expect that this library will be customized).
Therefore knowing that the patch is backwards-compatible - I am only adding new methods not modifying any existing ones - I will proceed with patch. The PR linked is a documentation of review process.
Comment Actions
Thanks for explaining your reasoning! It's a big patch, but I think the approach is okay. I'm guessing you tested on both native and web and confirmed that the changes to the minified bundle were necessary on web, and the changes to the source was necessary for native? (Or perhaps you kept those changes to make it easier for a reader to review)
Comment Actions
confirmed that the changes to the minified bundle were necessary on web
In fact they are not necessary. What is necessary for web are changes in the dist/localforage.js folder (and just those changes). This patch was generated in the following way:
- Forked the repo.
- Changed the source
- Let @kamil review the source.
- Run npm test inside the repo - this changed all files in dist (including dist/localforage.js).
- Created PR from changes, merged and created a patch from merged branch.
That is how changes in source and in minified bundle appeared.
In the very first iteration of this diff I had the patch with changes in source (for review) and in dist/localforage.js (manually introduced). However after switching to fork and then from fork to patch again I took advantage of automatic dist/localforage.js generation.
However now I am starring to worry - what if changes to minified bundle are not backwards compatible? Perhaps we should remove the part of the patch referring to minified bundle.
Comment Actions
It sounds like you're saying the minified bundle is not used at all. If that's the case, why would it lead to backwards compatibility issues?
Or is it the case that (for instance) we only use the minified bundle from web, but we only need the changes here for native?
In general, I'd lean towards removing the changes to the minified bundle if we're not using them / we don't need them.
Comment Actions
Perhaps I miscommunicated. I meant the case that the patch will fail to apply to bundle from updated version due to some conflicts. I don't expect them for source since I am just adding new functions but I wasn't sure hot it works for minified code. @tomek
told me that he would expect minified bundle to behave similarly as normal code.
Or is it the case that (for instance) we only use the minified bundle from web, but we only need the changes here for native?
We don't need any changes in this patch on native. It is just for web.
In general, I'd lean towards removing the changes to the minified bundle if we're not using them / we don't need them.
I will just check if it is not used if we are running production web build. If it is not we can remove it if you are leaning towards this.
Comment Actions
Thanks @marcin, that makes sense. To be honest, I would expect the minified bundle to be used in a production build, but I am not sure.
I meant the case that the patch will fail to apply to bundle from updated version due to some conflicts.
This case should be fairly visible – it will cause yarn cleaninstall to fail, so we will know that we have to address it.
Comment Actions
Accepting as I already reviewed this here: https://github.com/marcinwasowicz/localForage/pull/1.
Please check the minified bundle compatibility before landing.
| web/flow-typed/npm/localforage_v1.5.x.js | ||
|---|---|---|
| 79 | I'm sorry for not catching this earlier, but two things according to this suggestion:
| |
| 86 | nit: in TS types you have expectedSynchronizationValue?: string - which should be used? | |
| web/flow-typed/npm/localforage_v1.5.x.js | ||
|---|---|---|
| 79 |
Flow won't let that happen. The following line causes flow to complain: const x = await localforage.getMultipleItems<number>(['x'], 'y'); | |
| 86 | The flow ones. | |