reactivue
reactivue copied to clipboard
[POC] Remove inhouse `currentInstance` in favor of Vue's
Currently, we're duplicating Vue's currentInstance
reference. This is because Vue does not currently export setCurrentInstance
, which is noted here:
https://github.com/vuejs/vue-next/issues/4611
However, by using patch-package
, we can emulate a single-line code change in Vue's production environment to export said variable. By doing so we gain a few things:
- The ability to remove
currentInstance
and similar methods - We no longer need to manually track
effect
, since@vue/runtime-core
already does this when using it'scurrentInstance
- We can therefore remove our own
watch
implementation, as well as ourcomputed
implementation and more - We can utilize
effectScope
afterwards to shorten the codebase even further, removing thewindow
object and trackingscope
within components themselves (not in this PR)
This PR demonstrates what it might look like if we change 1-3 and removes ~200 lines of code. What's more, the code that's left is made significantly more straightforward as a result. While not a perfect metric, using the scc
code analysis tool, it estimates that our code will half its complexity.
patch-package only works in apps, while we are a library. We can't assume userland's vue version and we can't patch that for the users.
The patch-package concerns are understandable and are why it's listed as a proof of concept.
However, I may be misunderstanding - couldn't we use peerDeps to infer the user's Vue version?
Further, since we're telling users to alias Vue to reactivue currently, wouldn't that not be a concern? Since it's aliased, even our patch package would apply to them as well, so long as they're properly importing as our README suggests I think
Am I missing an aspect to my understanding? 😅
The patch basically changes the local file, where on the user side the deps are fetched from NPM fresh without the patch. I think it's too much to ask users to also patch their deps in order to use this lib.
Oh! Right, because it's simply a dependency that's built and not a bundled asset. I'm used to bundling a lot of the libraries I publish, so I often forget this is an option 😅
Since we bound current instance to React component by using useRef
I believe we can get rid of getNewInstanceId
, window
assignments and my HMR workarounds for StrictMode. We might need to do some checks inside createStore function, not sure. Also I believe if we store instances and/or uids with Context API we can make it more SSR friendly and eliminate side-effects.
My thoughts about the patch: If accessing setCurrentInstance
is enough for our needs I would say we should wait for vue-next. Otherwise we could manage our own fork or submodule/patch workflow, but this way could turn into pain very quickly. We should avoid that 😅