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: globalProperties config leak (#1187) #1202

Merged
merged 1 commit into from
Jan 6, 2022

Conversation

purepear
Copy link
Contributor

@purepear purepear commented Jan 1, 2022

This PR fixes #1187 by doing 1st-level-pseudo-deep-clone merge of Vue's AppConfig (only the props that we know are objects - globalProperties and compilerOptions).

I was tempted to use lodash's clondeDeep but didn't want to bring another dependency. (looked at lodash.clonedeep as well but the lodash.* packages are not updated anymore). Not loving how mergeAppConfig() turned out but it does the job without additional dependencies. Open to alternatives :)

This might break existing tests if they rely on the buggy behavior but i don't think it should be considered breaking changes 🤔

Copy of the original issue and repro:

Thanks for the great work, guys! 🙇 I just noticed that when i pass the router plugin in a test, then the rest of the tests were inheriting the router.

mount(Component, { global: { plugins: [router] }})

I also have a global config setup:

config.global.config.globalProperties = {
  $router: routerMock,
  $route: routeMock,
}

I believe it has sth to do with the config merging strategy - it uses shallow clone and in the case of deeply nested objects, it overwrites the VTU's global config object. This might not be limited to plugins/config/globalProperties but other parts of the config as well 🤔

Here's how to reproduce:

import { config, mount } from '@vue/test-utils'
import { h } from 'vue'

config.global.config.globalProperties = {
  $anotherThing: 'not used but sth to demonstrate a non-empty global config',
}

const Plugin = {
  install(app) {
    app.config.globalProperties.$something = 'something'
  },
}

const Component = {
  render() {
    return h('div', this.$something)
  },
}

test('should pass with plugin', () => {
  const wrapper = mount(Component, {
    global: {
      plugins: [Plugin],
    },
  })

  expect(wrapper.html()).toContain('something')
})

test('should pass without plugin but it fails!', () => {
  const wrapper = mount(Component)

  expect(wrapper.html()).not.toContain('something')
})

@lmiller1990
Copy link
Member

Seems fine... not sure if this will break anyone, as you pointed out, I guess we will find out.

Lodash mergeDeep does not treeshake well, it brings in about 1k loc or something to that tune. We are keeping this lib as small and dep free as possible, so thanks for this!

@lmiller1990
Copy link
Member

Can you please run prettier and repush?

@purepear
Copy link
Contributor Author

purepear commented Jan 5, 2022

Amended last commit after running perttier and then force-pushed. CI should be happy now :)

@lmiller1990 lmiller1990 self-requested a review January 6, 2022 22:32
@lmiller1990 lmiller1990 merged commit e5a331c into vuejs:master Jan 6, 2022
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.

Config leaks between tests
2 participants