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

Fix: 500 error using Vue+Meteor SSR after update vue-meta version to 2.3.4 #569

Closed
wants to merge 1 commit into from

Conversation

mrauhu
Copy link

@mrauhu mrauhu commented Jun 5, 2020

Greetings, @pimlie

This pull-request fixes the 500 error with document is not defined message when we using Vue Meta and Vue+Meteor SSR.

Problem is the Vue+Meteor SSR package doesn't have this.$el on hook:beforeMount, so the getTag() function is called in { ssr: true } context.

Bug was introduced in #563.

Best wishes,
Sergey.

@codecov
Copy link

codecov bot commented Jun 5, 2020

Codecov Report

Merging #569 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #569   +/-   ##
=======================================
  Coverage   98.68%   98.68%           
=======================================
  Files          33       33           
  Lines         684      684           
  Branches      211      211           
=======================================
  Hits          675      675           
  Misses          5        5           
  Partials        4        4           
Impacted Files Coverage Δ
src/shared/mixin.js 91.56% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 92350d5...5b0c2ad. Read the comment docs.

@mrauhu mrauhu changed the title Fix: 500 error using Vue Meteor SSR after update vue-meta version to 2.3.4 Fix: 500 error using Vue+Meteor SSR after update vue-meta version to 2.3.4 Jun 5, 2020
@pimlie
Copy link
Collaborator

pimlie commented Jun 7, 2020

@mrauhu Hey, thanks for this PR. Could you please create a reproduction of the issue you are experiencing?

After reading your explanation it doesnt become immediately clear unfortunately what you are trying to solve. You mention this.$el but that is used before your proposed change. Furthermore you are checking document but that should always be available afaik as the beforeMount lifecycle hook should only run on the client.

Thanks!

@mrauhu
Copy link
Author

mrauhu commented Jun 7, 2020

@pimlie I'm created the repository with code and instructions for reproduction of the bug:
https://github.com/mrauhu/meteor-ssr-vue-meta

Thank you for participating.

@pimlie
Copy link
Collaborator

pimlie commented Jun 7, 2020

@mrauhu Thanks for the repro. I have little xp with Meteor but why does the beforeMount / mounted Vue lifecycle hook run at all when there is no global document available?

The Vue SSR docs state that these methods dont run during SSR, so why do they run for Meteor?

@mrauhu
Copy link
Author

mrauhu commented Jun 7, 2020

I'm updated the repository, and added logs for Vue lifecycle hooks:

First run

On a server reload after change any *.js or *.vue file.

Server

[SSR] /
server beforeCreate()
server created()
server beforeMount()
(STDERR) [Vue warn]: Error in event handler for "hook:beforeMount": "ReferenceError: document is not defined"
(STDERR)
(STDERR) (found in <Root>)
(STDERR) ReferenceError: document is not defined
(STDERR)     at getTag (C:\Work\nuxt\meteor-ssr-vue-meta\node_modules\vue-meta\dist\vue-meta.common.js:403:17)
(STDERR)     at Vue.<anonymous> (C:\Work\nuxt\meteor-ssr-vue-meta\node_modules\vue-meta\dist\vue-meta.common.js:525:27)
Log with timestamps and full stack trace
I20200608-01:32:57.821(3)? [SSR] /
I20200608-01:32:57.827(3)? server beforeCreate()
I20200608-01:32:57.828(3)? server created()
I20200608-01:32:57.829(3)? server beforeMount()
W20200608-01:32:57.830(3)? (STDERR) [Vue warn]: Error in event handler for "hook:beforeMount": "ReferenceError: document is not defined"
W20200608-01:32:57.830(3)? (STDERR)
W20200608-01:32:57.831(3)? (STDERR) (found in <Root>)
W20200608-01:32:57.847(3)? (STDERR) ReferenceError: document is not defined
W20200608-01:32:57.848(3)? (STDERR)     at getTag (C:\Work\nuxt\meteor-ssr-vue-meta\node_modules\vue-meta\dist\vue-meta.common.js:403:17)
W20200608-01:32:57.849(3)? (STDERR)     at Vue.<anonymous> (C:\Work\nuxt\meteor-ssr-vue-meta\node_modules\vue-meta\dist\vue-meta.common.js:525:27)
W20200608-01:32:57.850(3)? (STDERR)     at Vue.on (C:\Work\nuxt\meteor-ssr-vue-meta\node_modules\vue\dist\vue.runtime.common.dev.js:3813:10)
W20200608-01:32:57.851(3)? (STDERR)     at invokeWithErrorHandling (C:\Work\nuxt\meteor-ssr-vue-meta\node_modules\vue\dist\vue.runtime.common.dev.js:1850:26)
W20200608-01:32:57.852(3)? (STDERR)     at Vue.$emit (C:\Work\nuxt\meteor-ssr-vue-meta\node_modules\vue\dist\vue.runtime.common.dev.js:3876:9)
W20200608-01:32:57.854(3)? (STDERR)     at callHook (C:\Work\nuxt\meteor-ssr-vue-meta\node_modules\vue\dist\vue.runtime.common.dev.js:4211:8)
W20200608-01:32:57.855(3)? (STDERR)     at mountComponent (C:\Work\nuxt\meteor-ssr-vue-meta\node_modules\vue\dist\vue.runtime.common.dev.js:4031:3)
W20200608-01:32:57.856(3)? (STDERR)     at Vue.$mount (C:\Work\nuxt\meteor-ssr-vue-meta\node_modules\vue\dist\vue.runtime.common.dev.js:8392:10)
W20200608-01:32:57.857(3)? (STDERR)     at Vue._init (C:\Work\nuxt\meteor-ssr-vue-meta\node_modules\vue\dist\vue.runtime.common.dev.js:4999:10)
W20200608-01:32:57.862(3)? (STDERR)     at new Vue (C:\Work\nuxt\meteor-ssr-vue-meta\node_modules\vue\dist\vue.runtime.common.dev.js:5065:8)
W20200608-01:32:57.863(3)? (STDERR)     at createApp (imports/ui/createApp.js:56:15)
W20200608-01:32:57.864(3)? (STDERR)     at imports/startup/server/vue-ssr.js:17:38
W20200608-01:32:57.865(3)? (STDERR)     at new Promise (<anonymous>)
W20200608-01:32:57.865(3)? (STDERR)     at Object.VueSSR.createApp (imports/startup/server/vue-ssr.js:15:12)
W20200608-01:32:57.866(3)? (STDERR)     at packages/akryum:vue-ssr/server/index.js:97:31
W20200608-01:32:57.867(3)? (STDERR)     at Meteor.EnvironmentVariable.EVp.withValue (packages\meteor.js:1234:12)
W20200608-01:32:57.867(3)? (STDERR)     at packages/akryum:vue-ssr/server/index.js:87:23
W20200608-01:32:57.870(3)? (STDERR)     at Meteor.EnvironmentVariable.EVp.withValue (packages\meteor.js:1234:12)
W20200608-01:32:57.870(3)? (STDERR)     at packages/akryum:vue-ssr/server/index.js:82:24
W20200608-01:32:57.872(3)? (STDERR)     at new Promise (<anonymous>)
W20200608-01:32:57.872(3)? (STDERR)     at packages/akryum:vue-ssr/server/index.js:74:20
W20200608-01:32:57.873(3)? (STDERR)     at packages/server-render/server-register.js:14:19

Client

client beforeCreate()
client created()
client beforeMount()
client mounted()
debug/init - vue-meta version ^2.3.4

Next runs

On page reload.

Server

[SSR] /
server beforeCreate()
server created()
server beforeMount()
server mounted()
server serverPrefetch()
debug/init - vue-meta version ^2.3.4
[SSR] Data prefetch: 9ms
Log with timestamps and full stack trace
I20200608-01:33:03.917(3)? [SSR] /
I20200608-01:33:03.919(3)? server beforeCreate()
I20200608-01:33:03.923(3)? server created()
I20200608-01:33:03.923(3)? server beforeMount()
I20200608-01:33:03.924(3)? server mounted()
I20200608-01:33:03.925(3)? server serverPrefetch()
I20200608-01:33:03.926(3)? debug/init - vue-meta version ^2.3.4
I20200608-01:33:03.926(3)? [SSR] Data prefetch: 9ms

Client

client beforeCreate()
client created()
client beforeMount()
client mounted()

@pimlie my suggestion is instead of debugging the Vue+Meteor platform add one check for the document:

if (typeof document !== 'undefined' &&

@pimlie pimlie requested review from atinux and TheAlexLichter June 10, 2020 09:57
@pimlie
Copy link
Collaborator

pimlie commented Jun 10, 2020

@atinux @manniL What do you think, should we merge this? Its not a big fix, but still it seems wrong to fix something that shouldn't happen in the first place. The Vue SSR docs clearly state the mounted hooks shouldnt run, but for some reason they do with Meteor.

Im slightly leaning to not merging until we know why Meteor behaves differently then the Vue SSR docs specify as it could just be an upstream bug in Meteor

@atinux
Copy link
Member

atinux commented Jun 10, 2020

Agree @pimlie

@chris-visser
Copy link

You sure you want to merge? I can take a look at the Meteor Vue package and see if I can fix it there. If this makes it into nuxt it might lead to other people with problems.

@pimlie
Copy link
Collaborator

pimlie commented Jun 12, 2020

@chris-visser If you could have a look that would be awesome!

I have zero to none xp with meteor, so maybe it is correct that mounted hooks runs on ssr and we should just merge this. But if not then either there is a bug in vue-meteor or a bug/config issue in the reproduction. I did check the vue-meteor-ssr example (which it seems the repro is based on) and I didnt notice anything wrong.

@chris-visser
Copy link

If its a Meteor Vue SSR bug, its fixable via Meteor Vue and it will be there within a very short time. I would not recommend merging this

@mrauhu
Copy link
Author

mrauhu commented Jun 12, 2020

@chris-visser thank you for debugging.

@chris-visser
Copy link

I have been diving trough the Vue code, but there seems to be nothing that prevents the beforeMount hook from triggering on the server:

https://github.com/vuejs/vue/blob/dev/src/core/instance/lifecycle.js#L141

export function mountComponent (
  vm: Component,
  el: ?Element,
  hydrating?: boolean
): Component {
  vm.$el = el
  if (!vm.$options.render) {
    vm.$options.render = createEmptyVNode
    if (process.env.NODE_ENV !== 'production') {
      /* istanbul ignore if */
      if ((vm.$options.template && vm.$options.template.charAt(0) !== '#') ||
        vm.$options.el || el) {
        warn(
          'You are using the runtime-only build of Vue where the template ' +
          'compiler is not available. Either pre-compile the templates into ' +
          'render functions, or use the compiler-included build.',
          vm
        )
      } else {
        warn(
          'Failed to mount component: template or render function not defined.',
          vm
        )
      }
    }
  }
  callHook(vm, 'beforeMount')

If this is true, then maybe it is valid to accept this PR and that would fix it for Meteor too. However, maybe its just better to check if this.$el is defined?

@mrauhu
Copy link
Author

mrauhu commented Jun 13, 2020

However, maybe its just better to check if this.$el is defined?

@chris-visser there is already check for this.$el:

wasServerRendered = this.$el && this.$el.nodeType === 1 && this.$el.hasAttribute('data-server-rendered')

And this check do nothing with an undefined document.

@chris-visser
Copy link

chris-visser commented Jun 13, 2020

What I mean is that the Vue instance does this:

  el = el && inBrowser ? query(el) : undefined
  return mountComponent(this, el, hydrating)

The inBrowser var in Vue is just typeof window === undefined. When I check this in Meteor on the Server (just to be sure) its indeed undefined. This means that el is also undefined, which is the value for this.$el in this mixin. So based on that, I would guess that the check in this hook is wrong:

The below code would return a falsey value, because this.$el is on the server always undefined. That means that wasServerRendered is incorrect, because on the server this value is never true.

wasServerRendered = this.$el && this.$el.nodeType === 1 && this.$el.hasAttribute('data-server-rendered')

Any check for the opposite would of course result in exactly the opposite - which is the bug that you now have:

!wasServerRendered && $root[rootConfigKey] && $root[rootConfigKey].appId === 1

This one results in true on the server: !wasServerRendered

@stale
Copy link

stale bot commented Jul 4, 2020

Thanks for your contribution to vue-meta! This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you would like this issue to remain open:

  1. Verify that you can still reproduce the issue in the latest version of vue-meta
  2. Comment the steps to reproduce it
    Issues that are labeled as pending will not be automatically marked as stale.

@stale stale bot added the stale label Jul 4, 2020
@stale stale bot closed this Jul 11, 2020
@mrauhu
Copy link
Author

mrauhu commented Jul 11, 2020

@pimlie, @atinux please, open this pull request.

Unfortunately, the vue-meta from version 2.3.4 and up is broken for using with Vue+Meteor integration.

@pimlie
Copy link
Collaborator

pimlie commented Jul 12, 2020

@chris-visser Thanks for your time looking into this!

And sorry for the late response, but its still unclear why the beforeMount hook is even run on the server.

@mrauhu I am very sorry but I dont want to merge a fix in, however simple it is, just because we didnt uncover the real cause of the issue. Again, this section: https://ssr.vuejs.org/guide/universal.html#component-lifecycle-hooks of the Vue SSR docs clearly state that beforeMount should only run on the client and isnt executed on the server.

So I dont know whats going wrong here but it seems there is either a bug in vue, vue-server-renderer or vue-meteor? Although if this was a general vue or vue-server-renderer issue then I would have expected that we had also seen this issue with Nuxt.js, but as we haven't my guess is still that this is a vue-meteor issue.

These beforeMount hooks and the var wasServerRendered dont matter at all on the server and just shouldn't be touched. This code only exists to work-around a hydration issue on the client.

The only recommenddation I can give you is to try if you can narrow it down by trying the following things:

  • Add a SFC with a beforeMount hook, see if this is executed on the server:
export default {
  beforeMount() {
    console.log('SFC beforeMount')
  }
  • Create a mixin with a beforeMount hook as previously
export default {
  mixins: [myMixinWithBeforeMountHook]
  • Add a SFC with a beforeCreate hook in which you call
export default {
  beforeCreate() {
    this.$once('hook:beforeMount', () => console.log('SFC beforeCreate listen'))
  • Create a mixin with a beforeCreate hook as previously
export default {
  mixins: [myMixinWithBeforeCreateHookThatListensToBeforeMount]

If any of these are logging something on the server then it would be an indication of a vue-meteor issue.

@mrauhu
Copy link
Author

mrauhu commented Jul 13, 2020

@pimlie thank you for the recommendation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants