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(runtime-core): align the merge strategies of component options with the behavior of vue2 #3600

Closed
wants to merge 5 commits into from
Closed

fix(runtime-core): align the merge strategies of component options with the behavior of vue2 #3600

wants to merge 5 commits into from

Conversation

HcySunYang
Copy link
Member

@HcySunYang HcySunYang commented Apr 14, 2021

Fix: #3566, #2791

Still WIP, points that need to be optimized:

  • improve the type
  • more test cases

After the compat branch is merged, I need to re-work on this

@HcySunYang HcySunYang changed the title wip: align the merge strategies of component options with the behavior of vue2 fix(runtime-core): align the merge strategies of component options with the behavior of vue2 Apr 16, 2021
@HcySunYang HcySunYang added the 🔨 p3-minor-bug Priority 3: this fixes a bug, but is an edge case that only affects very specific usage. label Apr 16, 2021
@yyx990803
Copy link
Member

Great work in this PR - I tried to rebase it locally while figuring out a better caching strategy. We don't want to run options merging for every instance since this was a major perf overhead in Vue 2, but in Vue 3 caching becomes a bit tricky due to different app instances may have different global mixins and merge strategies.

The conflicts got a bit out of hand so I decided to start from a clean branch instead.

  • In 1e35a86 we are now caching merged options on the app context.

    • Note this removes the 3rd vm argument from merge strategy functions so that we can cache the merge result for all instances in the same app. This argument is technically unnecessary so I believe this is a worthwhile tradeoff.
  • e2ca67b is largely based on this PR (especially the tests)

    • Removed the watch test case because users should not rely on the merge result of watch callbacks.

This also ensures the behavior becomes more consistent between the compat build and the non-compat build.

@yyx990803 yyx990803 closed this Jun 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔨 p3-minor-bug Priority 3: this fixes a bug, but is an edge case that only affects very specific usage.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mixins are not merged correctly for built-in options in this.$options
3 participants