Skip to content

fix: record emitted events against each wrapper #354

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Feb 8, 2021
Merged

fix: record emitted events against each wrapper #354

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
26d91e4
fix: record emitted events against each wrapper
Feb 5, 2021
e32fe72
test: add emit test for class-style components
Feb 7, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 27 additions & 13 deletions src/emit.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,20 +1,27 @@
import { setDevtoolsHook, devtools } from 'vue'
import { setDevtoolsHook, devtools, ComponentPublicInstance } from 'vue'

const enum DevtoolsHooks {
COMPONENT_EMIT = 'component:emit'
}

let events: Record<string, unknown[]>
let componentEvents: Record<string, unknown[]>
let events: Record<string, typeof componentEvents>

export function emitted<T = unknown>(
vm: ComponentPublicInstance,
eventName?: string
): T[] | Record<string, T[]> {
const cid = vm.$.uid

const vmEvents = (events as Record<string, Record<string, T[]>>)[cid] || {}
if (eventName) {
const emitted = (events as Record<string, T[]>)[eventName]
const emitted = vmEvents
? (vmEvents as Record<string, T[]>)[eventName]
: undefined
return emitted
}

return events as Record<string, T[]>
return vmEvents as Record<string, T[]>
}

export const attachEmitListener = () => {
Expand All @@ -28,20 +35,27 @@ function createDevTools(events): any {
emit(eventType, ...payload) {
if (eventType !== DevtoolsHooks.COMPONENT_EMIT) return

// The first argument is root component
// The second argument is vm
// The third argument is event
// The fourth argument is args of event
recordEvent(events, payload[2], payload[3])
const [rootVM, componentVM, event, eventArgs] = payload
recordEvent(events, componentVM, event, eventArgs)
}
}

return devTools
}

function recordEvent(events, event, args) {
function recordEvent(events, vm, event, args): void {
// Functional component wrapper creates a parent component
let wrapperVm = vm
while (typeof wrapperVm.type === 'function') wrapperVm = wrapperVm.parent
Copy link
Member

@lmiller1990 lmiller1990 Feb 6, 2021

Choose a reason for hiding this comment

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

I wonder if this will behave weirdly with class components 🤔 those (might) also return type: 'function'.

What do non-functional component return for type?

Copy link
Contributor Author

@aethr aethr Feb 7, 2021

Choose a reason for hiding this comment

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

Just from observations coming out of the test suite (again I haven't tried this branch against my real project), non-functional components have an object type. Haven't tried class components (and not super familiar with them since our project doesn't use TS), but I can have a crack at putting a test case in for them after some research. :)

This part of the PR definitely felt a bit hacky, but unfortunately VTU seems to have 2x components in the hierarchy for functional components (they even have the same el reference). During the devtool emit hook, the vm argument is the inner component (functional) but when the user calls component.emitted() from the test, vm is the outer (object) component, so the $.uid is different.

I'm not that familiar with VTU or functional components so not sure about the deeper reasoning here, was just my naive attempt to bash out some code that gets the tests to pass. Very happy to take a different approach but might need a bit of guidance.

Copy link
Contributor Author

@aethr aethr Feb 7, 2021

Choose a reason for hiding this comment

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

Added a class-style component test and it seems fine. The type for class-style components is also object,

I'm sure this can be improved, happy to make any other changes!

Copy link
Member

@lmiller1990 lmiller1990 Feb 8, 2021

Choose a reason for hiding this comment

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

Cool, thanks. Don't worry about feeling hacky here, this code base has tons of weird hacks to do things you can't otherwise do (usually for good reason, like mutating props, intercept events, etc.

All the tests are green, no reason not to merge. I'll do a release at the end of the week, hoping to get a few more small fixes in.

Copy link
Contributor Author

@aethr aethr Feb 9, 2021

Choose a reason for hiding this comment

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

I had a thought, even though it's already merged I thought I'd put it out there.

Since we're discouraged to use $.uid, I considered whether VTU might generate it's own unique id for each Wrapper (tid or something), and during emit we could traverse up the hierarchy until we find a component with tid set. For most components this would be the initial vm but for functional it would likely be the parent.

This would be guaranteed to be a Wrapper, we could use the tid instead of $.uid for storage and retrieval, and we wouldn't be doing opaque "what type is this" checks that might be hard to understand in 6 months.

Copy link
Member

@lmiller1990 lmiller1990 Feb 9, 2021

Choose a reason for hiding this comment

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

sure, this sounds like a useful thing to have. do you want to make a PR? we could use something like nanoid. we are trying to keep this lib dependency free but you could just copy-paste it in: https://github.com/ai/nanoid/blob/main/index.js

or just math.random, up to you


const cid = wrapperVm.uid
if (!(cid in events)) {
events[cid] = {}
}
if (!(event in events[cid])) {
events[cid][event] = []
}

// Record the event message sent by the emit
events[event]
? (events[event] = [...events[event], [...args]])
: (events[event] = [[...args]])
events[cid][event].push(args)
}
2 changes: 1 addition & 1 deletion src/vueWrapper.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ export class VueWrapper<T extends ComponentPublicInstance> {
emitted<T = unknown>(): Record<string, T[]>
emitted<T = unknown>(eventName?: string): T[]
emitted<T = unknown>(eventName?: string): T[] | Record<string, T[]> {
return emitted(eventName)
return emitted(this.vm, eventName)
}

html() {
Expand Down
39 changes: 31 additions & 8 deletions tests/emit.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { defineComponent, FunctionalComponent, h, SetupContext } from 'vue'
import { Vue } from 'vue-class-component'

import { mount } from '../src'

Expand Down Expand Up @@ -77,10 +78,9 @@ describe('emitted', () => {
expect(wrapper.emitted().hello[1]).toEqual(['foo', 'bar'])
})

it('should propagate the original event', () => {
const Component = defineComponent({
name: 'ContextEmit',

it('should not propagate child events', () => {
const Child = defineComponent({
name: 'Child',
setup(props, { emit }) {
return () =>
h('div', [
Expand All @@ -91,21 +91,26 @@ describe('emitted', () => {

const Parent = defineComponent({
name: 'Parent',

setup(props, { emit }) {
return () =>
h(Component, { onHello: (...events) => emit('parent', ...events) })
h(Child, { onHello: (...events) => emit('parent', ...events) })
}
})
const wrapper = mount(Parent)
const childWrapper = wrapper.findComponent(Child)

expect(wrapper.emitted()).toEqual({})
expect(wrapper.emitted().hello).toEqual(undefined)
expect(childWrapper.emitted()).toEqual({})

wrapper.find('button').trigger('click')
expect(wrapper.emitted().parent[0]).toEqual(['foo', 'bar'])
expect(wrapper.emitted().hello).toEqual(undefined)
expect(childWrapper.emitted().hello[0]).toEqual(['foo', 'bar'])

wrapper.find('button').trigger('click')
expect(wrapper.emitted().parent[1]).toEqual(['foo', 'bar'])
expect(wrapper.emitted().hello).toEqual(undefined)
expect(childWrapper.emitted().hello[1]).toEqual(['foo', 'bar'])
})

it('should allow passing the name of an event', () => {
Expand Down Expand Up @@ -133,7 +138,7 @@ describe('emitted', () => {
expect(wrapper.emitted('hello')).toHaveLength(2)
})

it('gives a useful warning for functional components', () => {
it('captures events emitted by functional components', () => {
const Component: FunctionalComponent<
{ bar: string; level: number },
{ hello: (foo: string, bar: string) => void }
Expand All @@ -155,6 +160,24 @@ describe('emitted', () => {
expect(wrapper.emitted('hello')[0]).toEqual(['foo', 'bar'])
})

it('captures events emitted by class-style components', () => {
// Define the component in class-style
class Component extends Vue {
bar = 'bar'
render() {
return h(`h1`, {
onClick: () => this.$emit('hello', 'foo', this.bar)
})
}
}

const wrapper = mount(Component, {})

wrapper.find('h1').trigger('click')
expect(wrapper.emitted('hello')).toHaveLength(1)
expect(wrapper.emitted('hello')[0]).toEqual(['foo', 'bar'])
})

it('captures an event emitted in setup', () => {
const Comp = {
setup(_, { emit }) {
Expand Down