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(provide): Merges symbol provides #7926

Merged
merged 3 commits into from
Dec 21, 2018
Merged

Conversation

privatenumber
Copy link
Contributor

Fixes merging multiple provides using Symbols

fix #7923

What kind of change does this PR introduce? (check at least one)

  • Bugfix
  • Feature
  • Code style update
  • Refactor
  • Build-related changes
  • Other, please describe:

Does this PR introduce a breaking change? (check one)

  • Yes
  • No

If yes, please describe the impact and migration path for existing applications:

The PR fulfills these requirements:

If adding a new feature, the PR's description includes:

  • A convincing reason for adding this feature (to avoid wasting your time, it's best to open a suggestion issue first and wait for approval before working on it)

Other information:
The test I added only passes in an environment with symbols (since it's handling a case that fails with symbols). This fails when the test is executed with Phantom, since it doesn't have Symbol support natively or with a polyfill, but passes in Chrome.

I can add babel-polyfill but seemed excessive for one test. Wanted to know how to move forward with that one before making any drastic changes to making it work.

Fixes merging multiple provides using Symbols

fix vuejs#7923

const keys = hasSymbol
? Reflect.ownKeys(from).filter(key => {
/* istanbul ignore next */
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wasn't entirely sure if this was needed but took it from inject.js to be safe.

@posva
Copy link
Member

posva commented Apr 26, 2018

Thanks for the contribution and sorry for the delay! I moved the test so it isn't run if Reflect isn't supported

@privatenumber
Copy link
Contributor Author

@posva Oh nice, thanks!

@yyx990803
Copy link
Member

Note: Reflect.ownKeys + filter with descriptor check is about 10x slower than Object.keys(), I think we need to avoid this cost for normal data merging, and only do this for provide() merges.

@yyx990803 yyx990803 added this to Todo in 2.6 Dec 5, 2018
@yyx990803 yyx990803 moved this from Todo to In progress in 2.6 Dec 20, 2018
@yyx990803 yyx990803 changed the base branch from dev to 2.6 December 21, 2018 17:48
@yyx990803 yyx990803 merged commit 1933ee8 into vuejs:2.6 Dec 21, 2018
2.6 automation moved this from In progress to Done Dec 21, 2018
f2009 pushed a commit to f2009/vue that referenced this pull request Jan 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
2.6
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants