reactivue icon indicating copy to clipboard operation
reactivue copied to clipboard

[POC] Remove inhouse `currentInstance` in favor of Vue's

Open crutchcorn opened this issue 2 years ago • 5 comments

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:

  1. The ability to remove currentInstance and similar methods
  2. We no longer need to manually track effect, since @vue/runtime-core already does this when using it's currentInstance
  3. We can therefore remove our own watch implementation, as well as our computed implementation and more
  4. We can utilize effectScope afterwards to shorten the codebase even further, removing the window object and tracking scope 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.

crutchcorn avatar Jan 08 '22 10:01 crutchcorn

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.

antfu avatar Jan 13 '22 06:01 antfu

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? 😅

crutchcorn avatar Jan 13 '22 06:01 crutchcorn

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.

antfu avatar Jan 13 '22 07:01 antfu

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 😅

crutchcorn avatar Jan 13 '22 07:01 crutchcorn

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 😅

sibbng avatar Jan 13 '22 16:01 sibbng