fix(runtime-core): use separate emits caches for components and mixins #11661
Conversation
Size ReportBundles
Usages
|
@vue/compiler-core
@vue/compiler-dom
@vue/compiler-ssr
@vue/compiler-sfc
@vue/reactivity
@vue/runtime-core
@vue/runtime-dom
@vue/server-renderer
@vue/shared
vue
@vue/compat
commit: |
gcaaa31928
force-pushed
the
emits-cache
branch
from
1caf05b
to
50ba141
Compare
|
/ecosystem-ci run |
|
📝 Ran ecosystem CI: Open
|
edison1105
added
🍰 p2-nice-to-have
gcaaa31928
force-pushed
the
emits-cache
branch
from
50ba141
to
f69841f
Compare
| @@ -599,3 +600,76 @@ describe('component: emit', () => { | |||
| expect(renderFn).toHaveBeenCalledTimes(1) | |||
| }) | |||
| }) | |||
|
|
|||
| test('merging emits for a component that is also used as a mixin', () => { | |||
There was a problem hiding this comment.
These tests are outside the describe block. All the other tests in this file are inside the describe. Is there a reason to have these outside?
| expect(`event validation failed for event "one"`).not.toHaveBeenWarned() | ||
| }) | ||
|
|
||
| test('merging props from global mixins and extends', () => { |
There was a problem hiding this comment.
| test('merging props from global mixins and extends', () => { | |
| test('merging emits from global mixins and extends', () => { |
|
|
||
| const CompB = { | ||
| mixins: [mixin, CompA], | ||
| created(this: ComponentPublicInstance) { |
There was a problem hiding this comment.
Couldn't this use defineComponent, rather than explicitly typing the this value? That seems to be how the other tests in this file are handling it.
There's some similar code in the other new test where the typings could just be removed.
| expect(renderProxy._.emitsOptions).toMatchObject(emits) | ||
| expect(extendedRenderProxy._.emitsOptions).toMatchObject(emits) |
There was a problem hiding this comment.
Is there a reason to grab things from internal properties here? Can't this be tested via the public API, like in the other test?
There was a problem hiding this comment.
Indeed, this part is redundant, so I have removed this.
|
|
||
| const app = createApp({ | ||
| render() { | ||
| return [h(CompA), ', ', h(CompB)] |
There was a problem hiding this comment.
| return [h(CompA), ', ', h(CompB)] | |
| return [h(CompA), h(CompB)] |
Given we aren't checking the rendered output, I think it'd be clearer to omit the separator string.
|
|
||
| const root = nodeOps.createElement('div') | ||
| app.mount(root) | ||
| expect(`event validation failed for event "one"`).not.toHaveBeenWarned() |
There was a problem hiding this comment.
Perhaps rather than using the warning to check for correctness we could check which validator functions are called? If the validators use vi.fn() we could test for toHaveBeenCalledTimes(1) and not.toHaveBeenCalled(), as appropriate?
gcaaa31928
force-pushed
the
emits-cache
branch
2 times, most recently
from
c508fc0
to
ce27cfd
Compare
@skirtles-code Thank you for your review ! I have adjusted the testing code according to your suggestions. Please take a look at it when you have time. |
gcaaa31928
force-pushed
the
emits-cache
branch
2 times, most recently
from
e370175
to
60dd480
Compare
gcaaa31928
force-pushed
the
emits-cache
branch
from
60dd480
to
6251055
Compare
edison1105
added
ready to merge
gcaaa31928
force-pushed
the
emits-cache
branch
from
6251055
to
18617c4
Compare
This seems to be the same issue as #11350, except that in this case, the emits cache is being reused by both mixins and components, which could lead to the emit validation being applied in the wrong context.
However, the fix here is the same: separate the emits cache from the appContext