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

WIP: Fix for issue 2261 #2264

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

Sandbagger
Copy link

@Sandbagger Sandbagger commented Nov 30, 2023

Work in progress

#2261

Cause:

  • ssrUtils is not available in the esm-browser version of Vue. However, Vue Test Utils uses it for renderToString.

Fix

Notes

Copy link

netlify bot commented Nov 30, 2023

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

Name Link
🔨 Latest commit 9b30a2b
🔍 Latest deploy log https://app.netlify.com/sites/vue-test-utils-docs/deploys/657abec05ea2e400082a7719
😎 Deploy Preview https://deploy-preview-2264--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 configuration.

src/index.ts Outdated
import { MountingOptions } from './types'
import { RouterLinkStub } from './components/RouterLinkStub'
import { createWrapperError } from './errorWrapper'
import { config } from './config'
import { flushPromises } from './utils/flushPromises'
import { enableAutoUnmount, disableAutoUnmount } from './utils/autoUnmount'

// is __SSR__ avaialble? If so, preferable to use that?
Copy link
Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

I don't know what's best. Maybe a variable defined in the rollup config would make more sense.

@lmiller1990 I think you were the one who set up the build config. Do you have an opinion on this?

Copy link
Member

Choose a reason for hiding this comment

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

I'd expect we want to mirror what core does where possible!

Copy link
Member

Choose a reason for hiding this comment

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

Either way, if it works, and we've got a test, happy to move forward with this. Did you want to make a change @Sandbagger? Any opinion @cexbrayat ?

Copy link
Member

Choose a reason for hiding this comment

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

Vue core uses a build variable, so I think it would be better to mirror that if @Sandbagger feels like trying this. Otherwise I'm OK with merging this (once the comment is removed)

Copy link
Author

Choose a reason for hiding this comment

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

Thanks chaps. I'll give rollup config variable a go.

src/index.ts Outdated
import { MountingOptions } from './types'
import { RouterLinkStub } from './components/RouterLinkStub'
import { createWrapperError } from './errorWrapper'
import { config } from './config'
import { flushPromises } from './utils/flushPromises'
import { enableAutoUnmount, disableAutoUnmount } from './utils/autoUnmount'

// is __SSR__ avaialble? If so, preferable to use that?
Copy link
Member

Choose a reason for hiding this comment

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

I'd expect we want to mirror what core does where possible!

src/index.ts Outdated
import { MountingOptions } from './types'
import { RouterLinkStub } from './components/RouterLinkStub'
import { createWrapperError } from './errorWrapper'
import { config } from './config'
import { flushPromises } from './utils/flushPromises'
import { enableAutoUnmount, disableAutoUnmount } from './utils/autoUnmount'

// is __SSR__ avaialble? If so, preferable to use that?
Copy link
Member

Choose a reason for hiding this comment

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

Either way, if it works, and we've got a test, happy to move forward with this. Did you want to make a change @Sandbagger? Any opinion @cexbrayat ?

@Sandbagger
Copy link
Author

@cexbrayat @cexbrayat I need to put a little bit more thought into using a build variable and how best to test it. I will spend some time over the weekend to do this with a view to opening another MR.

In the meantime would you both be happy if we get this in (I've removed the comment)?

@cexbrayat
Copy link
Member

@Sandbagger You can take your time and polish the PR no worries. I prefer to merge the final one 👍

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

3 participants