core icon indicating copy to clipboard operation
core copied to clipboard

fix(runtime-core): allow using `getCurrentInstance` with multiple builds of vue

Open pi0 opened this issue 2 years ago • 8 comments

Context: Changes in this pull request, are an attempt to fix the root cause of a long-lasting issue we had with Nuxt 3, composition API, and especially during the server-side-rendering phase but it can be related to any Vue SSR setup with the same situation. There is no single issue or reproduction I could refer to (1) but trying to explain as best as I can.

Vue 3 uses an internal singleton variable for CAPI that temporarily keeps the Vue instance while rendering components. it is used internally and can be accessed using getCurrentInstance() from other compostables.

This works perfectly fine as long as both user and all libraries import getCurrentInstance() from exactly the same file && the same evaluated instance. If for any reason they (or an intermediate composable) is not, the whole CAPI API will be broken with untraceable errors such as Server rendering context not provided or Cannot read property something of undefined.

There could be many reasons for this situation to happen:

  • Package managers hoist and install different versions of Vue for libraries, frameworks and user
  • Same version of Vue from different node_modules directories (when npm linking)
  • Same version of Vue with different builds (Vite picks esm-bundler, externalized dependencies pick cjs build)
  • Same version and build of Vue with different instances (One can be inlined by bundler and one externalized)
  • Vue and @vue/runtime-core (however it didn't happen, but I noticed vue package also has a standalone build of runtime-core)

The proposed changes fix this issue by storing temporary variables in a context stored in globalThis instead of local scope. This allows to getCurrentInstance() to work no matter from which version/build/format or instance as long as being in the same tick, the instance is available. It doesn't add perf overhead and wouldn't break concurrency constraints too since we unset it with the same strategy as before. An additional fix I made was is to use Symbol.for for ssrContext injection, deduplicating it using the global Symbol registry otherwise, two Symbol instances differ. (2)


Updates:

  • Using shared current instance is opt-in with internal API globalThis.__vue__. When it is set, vue uses this object over package local instance.

Notes:

(1) Linked issues: There were many older issues we solved with different methods in Nuxt 3 before. I finally made and verified it in order to fix the root cause of https://github.com/nuxt/nuxt/issues/18563. If needed, happy will list more relevant issues.

(2) Fixes to the core of CAPI for this, are impossible to be done at the userland or framework level. However, if for any reason you are against using globalThis, we can make it opt-in by optionally using globalThis if set by the framework and then falling back to a local variable.

pi0 avatar Jan 27 '23 20:01 pi0

Deploy Preview for vuejs-coverage failed.

Name Link
Latest commit ca3c029250c2d09f365baf090f5cb4afe39294d8
Latest deploy log https://app.netlify.com/sites/vuejs-coverage/deploys/63d43927a99a580008524915

netlify[bot] avatar Jan 27 '23 20:01 netlify[bot]

Deploy Preview for vuejs-coverage failed.

Name Link
Latest commit c52939698a2e0610dd06e752743b225dfefb8218
Latest deploy log https://app.netlify.com/sites/vuejs-coverage/deploys/63d4395a4e2304000883970b

netlify[bot] avatar Jan 27 '23 20:01 netlify[bot]

My concern with this is that it doesn't really address the the fundamental issue, since currentInstance isn't the only global singleton that the Vue runtime relies on. Some other examples include the reactivity tracking maps and built-in VNode type symbols like Text or Fragment. Only addressing currentInstance won't fix other similar issues caused by multiple copies of Vue being used.

Also, making it work with multiple copies mask the underlying problem, and can lead to users unknowingly ship multiple copies of Vue in the final bundler, which should be avoided.

So maybe a better direction for this is to introduce something that makes it easier to identify such cases.

yyx990803 avatar Feb 01 '23 09:02 yyx990803

Thanks for quick feedback @yyx990803

since currentInstance isn't the only global singleton that the Vue runtime relies on. Some other examples include the reactivity tracking maps and built-in VNode type symbols like Text or Fragment.

Indeed. But at least in Nuxt projects, currentInstance and SSR instance were major case of issues. (We might also extend this fix to other places if you think worth)

Also, making it work with multiple copies mask the underlying problem

I agree. However, this issue is mostly for development time because of externals and other factors being involved, we cannot optimize the Vue version. Rollup/Vite/Nitropack already deduplicate Vue version for production builds.

can lead to users unknowingly ship multiple copies of Vue in the final bundler, which should be avoided.

What do you think to explicitly throw an error in production builds of Vue dist if we detect such situation?

So maybe a better direction for this is to introduce something that makes it easier to identify such cases.

I agree ideally that would be the best. However in practice, solving many of those hoisting issues and dealing with them in dev, has only negative DX issues and is sometimes not easy to solve. I can make this behavior opt-in so that we apply it only for Nuxt.

pi0 avatar Feb 01 '23 10:02 pi0

Deploy Preview for vue-next-template-explorer failed.

Name Link
Latest commit 8a0083ce847974de05dea17afd1c72af4fda7490
Latest deploy log https://app.netlify.com/sites/vue-next-template-explorer/deploys/63e61a7ed818880008b65704

netlify[bot] avatar Feb 10 '23 09:02 netlify[bot]

Update: I have updated the tests. Also made this change completely opt-in via a private API. When globalThis.__vue_ctx__ is set, vue reuses the context over package-scoped context.

pi0 avatar Feb 10 '23 10:02 pi0

Can you check if f597dc69 + 8d2d5bf4 achieve the same end result in your reproductions? I wanted to centralize the logic so that the rest of the codebase can forget about this constraint.

yyx990803 avatar Feb 21 '23 14:02 yyx990803

Using warning and global setters, i think we will have less issues however:

  • We probably need to avoid showing this warning when same version of Vue is from different locations (external util linked from another node_modules directory -- only in development can happen)
  • This warning can be false-positive only if getCurrentInstance is imported from different and compatible version Vue by a library (an external library might have a lower/higher semver/minor-patch dependency and Node.js resolution algorithm picks it, however it is not actually used for runtime. Nuxt for example uses main version always)
  • ~~We probably also need to use Symbol.for for SSR injection key (this was second fix)~~

pi0 avatar Feb 21 '23 14:02 pi0

This feature has already been implemented in https://github.com/vuejs/core/commit/8d2d5bf48a24dab44e5b03cb8fa0c5faa4b696e3. Thank you for your contribution!

edison1105 avatar Oct 17 '24 07:10 edison1105