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

revert(#434): use getSlidesVNodes utility to extract slides to be clone #443

Closed
wants to merge 1 commit into from

Conversation

ismail9k
Copy link
Owner

@ismail9k ismail9k commented Dec 8, 2024

The change was found to cause issues during static site generation processes, particularly with frameworks like VitePress.

By reverting this, we restore compatibility with static file generation workflows while maintaining functionality for existing use cases.

Closes #442

 The change was found to cause issues during static site generation processes, particularly with frameworks like VitePress.
@ismail9k
Copy link
Owner Author

ismail9k commented Dec 8, 2024

Could you please review this PR @Tofandel?

@Tofandel
Copy link
Contributor

Tofandel commented Dec 8, 2024

Did you check that this would solve it? It looks to be more of a SSR issue, where the generated stuff SSR does not match what is generated on mounted, I'm not sure this would fix this

From what I see the cloned vNodes are not rendering properly SSR (before mount), they are comments

image

And because vue's hydration policy dictates that hydration mismatch should not update, they do not get mounted after that

@Tofandel
Copy link
Contributor

Tofandel commented Dec 8, 2024

I think we should disable the cloned vNode until the mount hook which would solve the hydration mismatch issue

I'll add a SSR test case to the test suite

@ismail9k
Copy link
Owner Author

ismail9k commented Dec 8, 2024

Yes, this solution resolves the issue. You can verify it by building the documentation, as the problem occurs specifically during the build process.

I don’t believe that disabling clones until the component is mounted would address the issue. From my observations, the carousel goes through two rendering phases:
1. Initial render: The slides are displayed without clones because they haven’t been registered yet.
2. Second render: The cloned slides are added after registration.

Since VitePress processes the HTML markup during static site generation, it only captures the initial render, which excludes the cloned slides. This discrepancy leads to the problem during build time.

@Tofandel Tofandel mentioned this pull request Dec 8, 2024
@Tofandel
Copy link
Contributor

Tofandel commented Dec 8, 2024

The problem with getSlidesVnodes was that it was not working properly with Slides passed through a slot, which was why #434 was needed

The reason is that from a slot we are not getting a Fragment, we are getting this

    type: {
      __name: 'Slides',
      props: [Object],
      setup: [Function: setup],
      render: [Function: _sfc_render],
      __file: '/home/tofandel/PhpstormProjects/vue3-carousel/tests/components/Slides.vue'
    },
    children: null,

I don’t believe that disabling clones until the component is mounted would address the issue. From my observations, the carousel goes through two rendering phases:

It's actually the only way to properly fix the discrepancy with SSR, clones should only be added in the nextTick after mount

Here is a test case, that will fail with getSlidesVNodes

import { mount } from '@vue/test-utils'
import { expect, it, describe, beforeAll, vi } from 'vitest'
import { createSSRApp, h } from 'vue'
import { renderToString } from 'vue/server-renderer'

import App from '../components/BasicApp.vue'
import SlottedApp from '../components/SlottedApp.vue'

describe('SSR Carousel', () => {
  it('renders server side properly', async () => {
    const consoleMock = vi.spyOn(console, 'warn').mockImplementation(() => undefined)

    const comp = {
      render() {
        return h('div', { id: 'app' }, h(App, {
          wrapAround: true,
          modelValue: 1,
          itemsToShow: 2,
        }))
      },
    }
    const app = createSSRApp(comp)
    const html = await renderToString(app)
    document.body.innerHTML = html
    const wrapper = await mount(comp, { attachTo: '#app' })

    expect(consoleMock).not.toHaveBeenCalled()
    expect(html).toMatchSnapshot()
    expect(wrapper.html()).toMatchSnapshot()
  })

  it('renders slotted server side properly', async () => {
    const consoleMock = vi.spyOn(console, 'warn').mockImplementation(() => undefined)

    const comp = {
      render() {
        return h('div', { id: 'app' }, h(SlottedApp, {
          wrapAround: true,
          slideNum: 5,
        }))
      },
    }
    const app = createSSRApp(comp)
    const html = await renderToString(app)
    document.body.innerHTML = html
    const wrapper = await mount(comp, { attachTo: '#app' })

    expect(consoleMock).not.toHaveBeenCalled()
    expect(html).toMatchSnapshot()
    expect(wrapper.html()).toMatchSnapshot()
  })
})

The second test should fail with hydration mismatch

I'm implementing a proper fix in #444 with those tests

@ismail9k
Copy link
Owner Author

ismail9k commented Dec 9, 2024

Closed in favor of #444

@ismail9k ismail9k closed this Dec 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cloned Slides Not Rendered Correctly
2 participants