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

Add renderToString method #435

Merged
merged 9 commits into from
Feb 20, 2018
Merged

Conversation

eddyerburgh
Copy link
Member

Vue.config.devtools = false

export default function renderToString (component: Component, options: Options = {}): string {
const renderer = require('vue-server-renderer').createRenderer()
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason why vue-server-renderer gets imported using CommonJS and not ESM?

Copy link
Member Author

Choose a reason for hiding this comment

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

ESM imports are hoisted to the top, so any ESM import will always be bundled.

vue-server-renderer relies on node modules, like path, which makes it difficult to compile with webpack. require is only run when the code runs, so putting it inside the renderToString means we only try and import vue-server-renderer when the renderToString function is run.

We should also add a warning that checks if the user is in node, and we can throw an error if they aren't to tell them renderToString needs to be run in node.

Copy link
Contributor

Choose a reason for hiding this comment

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

Clear explanation, thank you very much!

@eddyerburgh eddyerburgh merged commit da7aadc into vuejs:dev Feb 20, 2018
@eddyerburgh eddyerburgh deleted the add-render-method branch February 20, 2018 06:57
@dotnetCarpenter
Copy link

I just did a npm update and got npm WARN @vue/test-utils@1.0.0-beta.12 requires a peer of vue-server-renderer@2.x but none is installed. You must install peer dependencies yourself..

Could you please update the release notes to explain that we need to install vue-server-renderer because of the added renderToString method.

I did a git blame on the package.json to see why but I imagine most developers won't go through the trouble.

@eddyerburgh
Copy link
Member Author

@dotnetCarpenter thanks for finding this.

I've created an issue to remove it as a peerDependency—#440.

I think most users won't use renderToString, so we can handle the dependency ourselves, without throwing a warning for people who won't use the method

@dotnetCarpenter
Copy link

@eddyerburgh That was not what I intended. If you have an API that has renderToString it should work out of the box. Having vue-server-renderer as a peer dependency is good as it gives me the choice to install it either as a devDependecy or a dependency.

I have a laravel-mix project that does not use SSR, hence do not need vue-server-renderer and I got a nuxt project that already has vue-server-renderer. I prefer nuxt to handle versions of vue-server-renderer but in my laravel-mix project, I have installed it as a devDependecy because it is only relevant to my tests.

But if you want to put in the work to handle it in vue-test-utils, that is of course fine. But I was not aiming at that. 😄 I was just expecting a better explanation to why I have to install a package, I am not using. I think a small update to the release text will suffice.

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