fix: record emitted events against each wrapper #354
Conversation
lmiller1990
left a comment
lmiller1990
left a comment
There was a problem hiding this comment.
Good catch and fix. Are you able to see if there is any weirdness with class components (if there is and it wasn't introduced here, we can probably make a new issue to deal with those).
| 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 |
There was a problem hiding this comment.
I wonder if this will behave weirdly with class components 🤔 those (might) also return type: 'function'.
What do non-functional component return for type?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
2 participants
fix #352
Emitted events are recorded against their component uid, so the events retrieved by
emitted()doesn't contain child events.Updated one of the tests to probe for this as well.
First stab at any typescript so please let me know if I've committed terrible type sins. 🙏