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: record native events against each wrapper they bubble through #394

Merged
merged 4 commits into from
Feb 19, 2021
Merged

fix: record native events against each wrapper they bubble through #394

merged 4 commits into from
Feb 19, 2021

Conversation

aethr
Copy link
Contributor

@aethr aethr commented Feb 17, 2021

fix #391

This is pretty ugly on a few levels and I think it needs some cleaning up, but I think it addresses the issue. Maybe going back to the old mixin is a better idea for native events, I think I'll leave that up to you because you know the history a bit better.

I added a test that covers I think a basic exploration of the problem being solved.

Again, I'm an absolute TS novice, 🙏 for me.

Looking at this code again, do we need to add a way to clear events similar to jest's mockClear()?

Comment on lines +33 to +35

this.attachNativeEventListener()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perhaps this method should be exported from emit.ts with this as an argument, the style looks very different from anything else so I tried to add a private method instead. There is definitely some bleeding of concerns.

src/vueWrapper.ts Outdated Show resolved Hide resolved
Comment on lines +49 to +50
const vm = this.vm
if (!vm) return
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Calling this.element breaks in some circumstances that I had trouble debugging, for some reason findComponent creates Wrapper objects without a vm? Anyway, if there's no vm, I guess there's nothing to bind event listeners to.

Copy link
Member

Choose a reason for hiding this comment

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

I cannot imagine why this would happen 🤔 I will look at it

Copy link
Member

Choose a reason for hiding this comment

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

Oh it makes sense, functional components can be found with findComponent but they do not have a vm

most likely someone is going to ask about capturing emitted events from functional components. let's deal with it if and when it comes up 🤷

Copy link
Contributor Author

@aethr aethr Feb 18, 2021

Choose a reason for hiding this comment

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

The test I added has a functional component for the grandparent, which works because it captures events that bubbled to the element of the outer wrapper, I suppose. It might be different for functional components without their own native DOM element but I suppose in that case DOM events wouldn't expect to be captured?

Copy link
Member

@lmiller1990 lmiller1990 left a comment

Choose a reason for hiding this comment

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

Seems good!

@lmiller1990
Copy link
Member

lmiller1990 commented Feb 18, 2021

Seems we need more investigation - I ran #391 against this PR and it does not pass, I get a weird output:

● Input.vue › v-model work

    expect(received).toHaveProperty(path)

    Expected path: "compositionStart"
    Received path: []

    Received value: {"change": [[{"isTrusted": false}]], "input": [[{"isTrusted": false}]]}

      46 |     await input.trigger('compositionstart')
      47 |
    > 48 |     expect(wrapper.emitted()).toHaveProperty('compositionStart')
         |                               ^
      49 |     expect(valueRef.value).toBe('valueRef change')
      50 |
      51 |     await input.trigger('input')

      at Object.<anonymous> (packages/components/input/__tests__/input.spec.ts:48:31)

I am starting to think this problem is unique to compositionstart and compositionend. I wonder if jsdom does not implement these... but it was ok with the emitMixin method. What is going on! Click and other native events are working fine.

Okay found it... this is the problem:

      if (/^on/.test(key)) {

For some reason compositionstart etc are not included. It's okay, we can just inline the events, I guess, from here: https://github.com/eddyerburgh/dom-event-types/blob/88d56b293f18d21481641820dd4436ef987dff3c/dom-event-types.json#L118

@lmiller1990
Copy link
Member

lmiller1990 commented Feb 18, 2021

I found this fix. I cannot push to your branch. Are you able to enable the setting "let maintainers push commits" (not sure the exact name of the setting).

@aethr
Copy link
Contributor Author

aethr commented Feb 18, 2021

Sorry @lmiller1990 I don't see the "Allow maintainers..." option on the PR. Apparently it won't work on this PR because it's underneath an Organisation (isaacs/github#1681), next time I'll make sure it's a user repo.

I'll make the changes you requested, and if you want to keep riffing on it we can figure out another solution. Maybe I'll re-create it as a user fork.

@lmiller1990
Copy link
Member

Should be fine, I will give this one last test and merge it. If we need to make any tweaks we still have a few release candidates up our sleeve :) thanks for this!

@aethr
Copy link
Contributor Author

aethr commented Feb 18, 2021

Sounds good! Thanks for your help getting it over the line.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

input.trigger('compositionstart') not work
2 participants