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

refactor: use devtools realize emit #296

Merged
merged 8 commits into from
Jan 24, 2021
Merged

Conversation

cuixiaorui
Copy link
Contributor

for #292

@afontcu
Copy link
Member

afontcu commented Jan 18, 2021

Hi! 👋 Could we add a test to cover the scenario Lachlan outlined? #292 (comment)

Copy link
Member

@cexbrayat cexbrayat left a comment

Choose a reason for hiding this comment

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

Thx for the PR! I added a few comments

src/emitMixin.ts Outdated Show resolved Hide resolved
src/emitMixin.ts Outdated
export const attachEmitListener = (vm: ComponentPublicInstance) => {
let events: Record<string, unknown[]> = {}
;(vm as any).__emitted = events
// use devtools capture this "emit"
Copy link
Member

Choose a reason for hiding this comment

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

nit: use devtools to capture this "emit"

src/emitMixin.ts Outdated
}

function createDevTools(events) {
const devTools: any = {
Copy link
Member

Choose a reason for hiding this comment

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

Instead of any, it would be great to properly type this. Ain't this a DevtoolsHook type?
Maybe there are not exposed by vue-next though?

Copy link
Member

Choose a reason for hiding this comment

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

We can get it hackily (the interface itself is not available). not sure how beneficial it'll be:

import { devtools } from 'vue'

const blah: Partial<typeof devtools> = {
  emit: () => {}
}

May need to be a partial unless we want to define on and the other hooks (we could make them no-ops).

src/emitMixin.ts Outdated
function recordEvent(events, event, args) {
events[event]
? (events[event] = [...events[event], [...args]])
: (events[event] = [[...args]])
Copy link
Member

Choose a reason for hiding this comment

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

Some documentation of what this does would be great (it's not obvious at first read).

src/emitMixin.ts Outdated
}
}
console.warn = consoleWarnSave
Copy link
Member

Choose a reason for hiding this comment

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

OOC: do we still need this?

Copy link
Member

Choose a reason for hiding this comment

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

Probably not - I put this there originally because I did not realize Vue 3 now wants you to declare the components emits property, I think.

Copy link
Contributor Author

@cuixiaorui cuixiaorui Jan 19, 2021

Choose a reason for hiding this comment

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

Yes, I don't think I need to deal with "console.warn" anymore

users should be declared "emits" property

In fact, the current logical implementation has broken the "console.warn" intercept

@lmiller1990
Copy link
Member

This PR is a great way to head into release candidate stage - the more value we can get from what is built into Vue core, the better.

@cuixiaorui
Copy link
Contributor Author

  • I have removed the interception logic for "console.warn" and processed some test cases
  • Refactoring to your opinion

Copy link
Member

@cexbrayat cexbrayat left a comment

Choose a reason for hiding this comment

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

Thanks!

@lmiller1990
Copy link
Member

lmiller1990 commented Jan 22, 2021

@cuixiaorui I tried this spec:

  it('captures an event emitted in setup', () => {
    const Comp = {
      setup(_, { emit }) {
        emit('foo')
      }
    }
    const wrapper = mount(Comp)
    expect(wrapper.emitted().foo).toBeTruthy()
  })

Still failing. You said you got it working in this comment - was that the case?

Had a bit of a look - I think we will need some way to attach the devtools hooks that does not rely on vm existing.

I think this is probably fine to merge as-is, it's still better than mutating the getCurrentInstance().emit. We should still look into capturing events emitted in setup.

@cuixiaorui
Copy link
Contributor Author

@lmiller1990

sorry

I ignored the test case you mentioned before

The previous implementation really didn't pass

I reworked the implementation logic

Now all the tests have passed


Now that we are no longer dependent on the "vm", we can move the emit logic ahead of the "createApp"

This is a good solution :)

@lmiller1990
Copy link
Member

Great job @cuixiaorui, great contribution!

I'll do a release soon with this.

@lmiller1990 lmiller1990 merged commit 4d030bb into vuejs:master Jan 24, 2021
johnleider added a commit to vuetifyjs/vuetify that referenced this pull request Mar 3, 2021
v2.0.0-rc.0 had changes to emits that are breaking
tests. vuejs/test-utils#296
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.

4 participants