Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Suspended component + transition #5148

Closed
JobinJia opened this issue Dec 21, 2021 · 11 comments · Fixed by #10652
Closed

Suspended component + transition #5148

JobinJia opened this issue Dec 21, 2021 · 11 comments · Fixed by #10652
Labels
❗ p4-important Priority 4: this fixes bugs that violate documented behavior, or significantly improves perf. has workaround A workaround has been found to avoid the problem 🐞 bug Something isn't working scope: suspense scope: transition

Comments

@JobinJia
Copy link

JobinJia commented Dec 21, 2021

Version

3.2.26

Reproduction link

SFC

Steps to reproduce

  1. open link
  2. Observe the animation happening only for loading

What is expected?

  1. transition worked with suspense default slot
  2. and the normal component

What is actually happening?

  1. transition only work with suspense fallback slot, not the default slot
  2. the normal component (not top await) loss transition animation
@posva
Copy link
Member

posva commented Dec 22, 2021

Still unsure of why this is happening only with the router. I tried creating a version replicating what the router does but I still couldn't find it. I also noticed, the problem appears with flat routes too.

Any help on this one is welcome!

@edison1105
Copy link
Member

edison1105 commented Dec 31, 2021

@posva
if change the <style scoped> to <style> , it works fine. see https://codesandbox.io/s/white-river-3mf6q
here is no scopedId
image

@posva
Copy link
Member

posva commented Dec 31, 2021

Thanks @edison1105

Still works on SFC Playground 🤔 I think I got confused between working or not working, it does not work without vue router indeed and removing scoped seems to be a workaround.

@edison1105
Copy link
Member

edison1105 commented Jan 1, 2022

@posva
I found something. Maybe this is not the right fix. Just a hint here.

  • currentScopeId is null when createVNode from TopLevelAwait.vue in RouterView.
    image

  • when suspense resolved, we will cloned it in _createVNode because Component is a VNode. So we losed the scopedId
    image

  • If we reassign the value to the scopeId when clone it will solve the problem.
    image

  • Here is a difference, in SFC Playground. It is not a VNode, so each time createVnode is called, the scopeId is assigned the value.
    image

@posva posva added ❗ p4-important Priority 4: this fixes bugs that violate documented behavior, or significantly improves perf. 🐞 bug Something isn't working scope: suspense scope: transition has workaround A workaround has been found to avoid the problem labels Jan 3, 2022
@posva posva changed the title about suspense & router-view & transition Suspended component + transition Jul 18, 2022
@ferferga
Copy link
Contributor

ferferga commented Apr 4, 2024

@edison1105 @posva I was facing the same issue with reusable transitions so I made up another (much simpler in my opinion) playground to better reproduce it, until I found this opened issue.

Here's the link

You can reproduce this by checking how the data-v-* attributes are missing on Suspense's inner div.

I can confirm that with #5195 I'm still having the issue (I rebased that PR into main and still the same thing).

@ferferga
Copy link
Contributor

ferferga commented Apr 4, 2024

@edison1105 Another interesting finding: SFC link

I think this issue is basically solvable if Suspense passes down $attrs, so the data-v* attribute also gets passed down.

@edison1105
Copy link
Member

@ferferga
I'll take a closer look at it later.

@edison1105
Copy link
Member

edison1105 commented Apr 6, 2024

@ferferga

You can reproduce this by checking how the data-v-* attributes are missing on Suspense's inner div.

I've made a PR to fix this.

Another interesting finding: SFC link

I think this is as expected for now, when you switch to the DEV, you will see that there are warnings. but there is room for improvement.

@ferferga
Copy link
Contributor

ferferga commented Apr 6, 2024

@edison1105 I can confirm that #10652 fixes my issue. To further help you guys get this finally fixed, I ran the following tests in the original @JobinJia 's playground (I downloaded the project from the SFC, set the following package.json with the exact latest version dependencies and, after npm i, replaced the built files from every case below into the relevant node_modules/@vue and node_modules/vue folders):

package.json
{
  "name": "vite-vue-starter",
  "version": "0.0.0",
  "type": "module",
  "scripts": {
    "dev": "vite",
    "build": "vite build",
    "serve": "vite preview"
  },
  "dependencies": {
    "vue": "3.4.21"
  },
  "devDependencies": {
    "@vitejs/plugin-vue": "5.0.4",
    "vite": "5.2.8"
  }
}

So I think it's safe to say that #10652 deprecates #5195 as it's a more broad solution (and with less code).


As for the warning in DEV, my bad for not seeing it in playground (I usually run it in production by default). I agree this is probably something that needs discussion elsewhere, but my 2 cents here:

  1. The warning mentions that the component renders a Fragment or text nodes: that's not true and misleading (in this context), since Transition renders a single child (<Suspense>) and <Suspense> also renders a single child (<div>). There are no text nodes or Fragments involved!
  2. I think Suspense should act as an invisible boundary for the async dependency handling, not giving the feel that it's "in the middle" of things, so I think it makes sense that it passes everything down (but it's own props and emits).

As a further follow-up of my use case (hopefully it helps to understand why it's good to have), here's my reusable transition component. It accepts as a name prop a set of transitions that I had defined in it's own scoped CSS. Only relevant parts are present

<template>
  <component
    :is="getComponent()"
    class="j-transition"
    v-bind="mergeProps($props, $attrs)">
     <slot />
  </component>
</template>

<script setup lang="ts">
import { type TransitionProps, ... } from 'vue';
interface JTransitionProps extends Omit<TransitionProps, 'name'> {
  name?: 'fade' | 'rotated-zoom' | 'slide-y-reverse' | 'slide-x' | 'slide-x-reverse';
}
const props = withDefaults(defineProps<JTransitionProps>(), { name: 'fade' });
...
</script>

<style scoped>
.j-transition {
  transition-duration: .3s !important;
  transition-timing-function: cubic-bezier(0.4, 0, 0.2, 1) !important;
  transform-origin: center !important;
}

/** fade */
.fade-enter-from,
.fade-leave-to {
  opacity: 0 !important;
}

.fade-enter-active,
.fade-leave-active {
  transition-property: opacity !important;
}

... rest of the transitions are defined here
</style>

By having the .j-transition class separated, I don't need to duplicate the duration, timing and origin functions in every class transition. This works flawlessly for every other use case, but in my App.vue:

<template>
   <RouterView v-slot="{ Component, route }">
      <JTransition
        :name="route.meta.transition?.enter ?? defaultTransition"
        :mode="defaultTransitionMode">
        <Suspense @resolve="apploaded = true">
          <div
            :key="route.meta.layout"
            class="h-100">
            <component
              :is="getLayoutComponent(route.meta.layout)"
              :key="route.meta.layout">
              <JTransition
                :name="route.meta.transition?.enter ?? defaultTransition"
                :mode="defaultTransitionMode">
                <Suspense suspensible>
                  <div
                    :key="route.path"
                    class="h-100">
                    <component
                      :is="Component"
                      :key="route.path" />
                  </div>
                </Suspense>
              </JTransition>
            </component>
          </div>
        </Suspense>
      </JTransition>
    </RouterView>
</template>

In order to workaround this, I need to extract the j-transition class to global styles and manually add the classes to both <div> there, which:

  • Separates some of the transition logic from my component, affecting the codebase maintenance. Additionally, that means adding comments or docs about thiss difference of behaviour.
  • Further complicates the template of my App.vue

Hopefully with this insight it's clearer why I think this is such a good thing to have. If you prefer to keep further discussion of this elsewhere, please let me know.

@edison1105
Copy link
Member

edison1105 commented Apr 7, 2024

@ferferga Thank you for the information you provided.

So I think it's safe to say that #10652 deprecates #5195 as it's a more broad solution (and with less code).

#5195 was submitted by me two years ago. At that time, my understanding was incorrect, so #5195 is not the correct solution. I have decided to close #5195 and keep #10652.

I think Suspense should act as an invisible boundary for the async dependency handling, not giving the feel that it's "in the middle" of things, so I think it makes sense that it passes everything down (but it's own props and emits).

Agreed~
Personally, I believe it needs to support fallthroughAttrs. I think we should open a new issue to discuss and track it separately. WDYT?

@ferferga
Copy link
Contributor

@edison1105 I opened a RFC here given this might be considered by some people as an API change.

Please let me know if this is more fitting as an issue instead.
Thank you very much.

@github-actions github-actions bot locked and limited conversation to collaborators Sep 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
❗ p4-important Priority 4: this fixes bugs that violate documented behavior, or significantly improves perf. has workaround A workaround has been found to avoid the problem 🐞 bug Something isn't working scope: suspense scope: transition
Projects
None yet
4 participants