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

feat: renderToString implementation #1572

Closed
wants to merge 5 commits into from

Conversation

wobsoriano
Copy link
Contributor

Adds a renderToString function just like the function from VTU for Vue 2.

It currently uses the same mount function, but checks if renderToString is true and uses the renderToString function from vue/server-renderer.

Sample usage:

it('returns correct html when onServerPrefetch is used', async () => {
  const Component = defineComponent({
    template: '<div>{{ text }}</div>',
    setup() {
      const text = ref('')

      onServerPrefetch(() => {
        return new Promise((resolve) => {
          setTimeout(() => {
            text.value = 'Text content'
            resolve(true)
          }, 100)
        })
      })

      onMounted(() => {
        text.value = 'Text content'
      })

      return { text }
    }
  })

  const wrapper = await renderToString(Component)

  expect(wrapper).toBe('<div>Text content</div>')
})

TODO:

  • More tests
  • Types

@netlify
Copy link

netlify bot commented Jun 3, 2022

Deploy Preview for vue-test-utils-docs ready!

Name Link
🔨 Latest commit f8fc5a1
🔍 Latest deploy log https://app.netlify.com/sites/vue-test-utils-docs/deploys/6370b36d025b2a000804f858
😎 Deploy Preview https://deploy-preview-1572--vue-test-utils-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

Copy link
Member

@lmiller1990 lmiller1990 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good start. Since renderToString is async, and mount is not, I'm guessing we probably will actually need to re-work mount a little - right now we are casting to any for the return type, which doesn't seem ideal - I think we want to keep the type safety as much as possible.

I'm thinking we probably need to extract out about 90% of the mount contents (all the mounting options related stuff) and have two separate methods - I don't think renderToString: true will work as well as we thought - I had not realized renderToString from the server renderer was async.

Thoughts?

I'm thinking we probably match the V1 API where we can: https://v1.test-utils.vuejs.org/api/#rendertostring


const wrapper = await renderToString(Component)

expect(wrapper).toBe('<!--[--><div>foo</div><div>bar</div><!--]-->')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should use the matchInlineSnapshot matcher?

src/mount.ts Outdated
@@ -516,6 +518,10 @@ export function mount(
}
}

if (options?.renderToString) {
return baseRenderToString(app) as any
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be Promise<string>?

@wobsoriano
Copy link
Contributor Author

@lmiller1990 Yeah I think we probably need to move out the contents of mount to another function just like the createInstance here

@lmiller1990
Copy link
Member

Yep I agree. Happy to wait if you want to give this a try, I think it shouldn't be too bad - we have lots of tests to guide the refactor.

@wobsoriano
Copy link
Contributor Author

@lmiller1990 so I created a createInstance function, for now, that contains most of the mount function logic, then added a new renderToString function in another file. I copy-pasted their types and changed the return types for now.

@lmiller1990
Copy link
Member

Nice, this is looking good. I wonder if we can avoid the copy paste of the types, seems like a maintenance nightmare. I will give it a try sometime this week.

@wobsoriano wobsoriano changed the title feat: add renderToString implementation feat: renderToString implementation Jun 27, 2022
@lmiller1990
Copy link
Member

Sorry, I forgot about this. Will try to get some 👀 on it soon.

Copy link
Collaborator

@freakzlike freakzlike left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, but I have no experiance with SSR. The types should be refactored (if possible) at some time.

@lmiller1990
Copy link
Member

lmiller1990 commented Aug 23, 2022

Me either - and I don't have much bandwidth to look into SSR now, either.

I wonder if we just merge and release this under a patch to let people try it out?

@wobsoriano sorry I forgot about this PR -- happy to merge and release, just need a quick rebase and maybe a note in the docs about this API? At least we should document that this API exists maybe here. Not sure if you want to add some basic examples or an additional page in to the docs... I will leave that decision to you.

@wobsoriano
Copy link
Contributor Author

@lmiller1990 will research more about this and then finalize.

@lmiller1990
Copy link
Member

If you want to resolve the conflicts, happy to merge now and doc later - even just a one liner in the API section would be fine, just so people know it exists. You could also link to the SSR docs.

}
})

const wrapper = await renderToString(Component)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

won't it be confusing to have renderToString both in server-renderer and in VTU?
Maybe we could call it otherwise in VTU?

resolve(true)
}, 100)
})
})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

onServerPrefetch sets the text to Text content, and onMounted as well: which one is tested in this test? Maybe they should have different values, and we should check both in the test?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I can update this part to check if the rendered text is the text from pre-fetched data? Something like

it('returns correct html with pre-fetched data on server', async () => {
  function fakeFetch(text: string) {
    return new Promise<string>((resolve) => {
      setTimeout(() => {
        resolve(text)
      }, 100)
    })
  }

  const Component = defineComponent({
    template: '<div>{{ text }}</div>',
    setup() {
      const text = ref<string | null>(null)

      onServerPrefetch(async () => {
        text.value = await fakeFetch('onServerPrefetch')
      })

      onMounted(async () => {
        if (!text.value) {
          text.value = await fakeFetch('onMounted')
        }
      })

      return { text }
    }
  })

  const contents = await renderToString(Component)

  expect(contents).toBe('<div>onServerPrefetch</div>')
})

Basing on this one https://vuejs.org/api/composition-api-lifecycle.html#onserverprefetch

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I think that would be great 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated 👍🏼

@cexbrayat
Copy link
Member

@wobsoriano Would you mind rebasing the PR please? We'll take a final look and merge

@freakzlike
Copy link
Collaborator

Closed in favor of #1971

@freakzlike freakzlike closed this Mar 20, 2023
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.

None yet

4 participants