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(test-utils): fix cancelable attribute in dom events #1460

Merged
merged 1 commit into from Mar 20, 2020
Merged

fix(test-utils): fix cancelable attribute in dom events #1460

merged 1 commit into from Mar 20, 2020

Conversation

lcmen
Copy link
Contributor

@lcmen lcmen commented Mar 8, 2020

What kind of change does this PR introduce? (check at least one)

  • Bugfix
  • Feature
  • Code style update
  • Refactor
  • Build-related changes
  • Other, please describe:

Does this PR introduce a breaking change? (check one)

  • Yes
  • No

If yes, please describe the impact and migration path for existing applications:

The PR fulfills these requirements:

If adding a new feature, the PR's description includes:

  • A convincing reason for adding this feature (to avoid wasting your time, it's best to open a suggestion issue first and wait for approval before working on it)

Other information:

It's my first PR (big hello to everyone!) with a small fix to correctly assign cancelable attribute for events created via trigger(event_name) API in modern browsers.

I've tried to come with some tests for this bug:

// test/specs/create-dom-event.spec.js
import createDOMEvent from '../../packages/test-utils/src/create-dom-event'

describe('createDOMEvent', () => {
  it('returns event', () => {
    const event = createDOMEvent('click', {})
    expect(event.bubbles).to.equal(true)
    expect(event.cancelable).to.equal(true)
  })
})

But it turns out that tests are run in PhantomJS and I believe it uses createOldEvent instead of createEvent which is not affected by this bug.

PS. How could I import create-dom-event file in such test to avoid all those ../../ in the beginning of the file's path?

@lmiller1990
Copy link
Member

lmiller1990 commented Mar 10, 2020

Hi! Good work.

The tests are run in both a jsdom environment and karma (using phantomjs). You can see we have a function that will conditionally run a test depending whether it is phantomjs or not here. Does this bug not manifest in a jsdom env? What happens when you add the test?

You should be able to write import createDOMEvent from 'test-utils/src/create-dom-event' and it will figure out what the import means. Anything in "packages" can basically be imported from using an absolute path.

@lcmen
Copy link
Contributor Author

lcmen commented Mar 10, 2020

Thank you for feedback!

The tests are run in both a jsdom environment and karma (using phantomjs). You can see we have a function that will conditionally run a test depending whether it is phantomjs or not here. Does this bug not manifest in a jsdom env? What happens when you add the test?

Thanks for pointing in the right direction! Using itDoNotRunIf does work and I'm able to reproduce the bug in tests. I've added such test to the PR.

You should be able to write import createDOMEvent from 'test-utils/src/create-dom-event' and it will figure out what the import means. Anything in "packages" can basically be imported from using an absolute path.

Unfortunately it doesn't work. I'm getting a following error: Module not found: 'test-utils/src/create-dom-event' in '/Users/lucas/Workspace/Personal/github.com/lowski/vue-test-utils/test/specs'. Any idea how to solve it?

@lmiller1990
Copy link
Member

Hm, how are you running the tests? Are you using yarn test:unit:only:dev?

@lmiller1990 lmiller1990 self-requested a review March 10, 2020 22:14
@lcmen
Copy link
Contributor Author

lcmen commented Mar 11, 2020

Hm, how are you running the tests? Are you using yarn test:unit:only:dev?

Actually I was using npm run test or npm run test:unit but now I've just tried yarn test:unit:only:dev and import createDOMEvent from 'test-utils/src/create-dom-event' doesn't work either.

@lmiller1990
Copy link
Member

lmiller1990 commented Mar 13, 2020

Weird. I will pull your branch down this weekend and try it out and see if I can figure out what is going on.

edit: try import createDOMEvent from '@vue/test-utils/src/create-dom-event. It looks like we do that in other tests. You might need to run yarn build if you made changes in packages. Then yarn test:unit:only. You can also use --watch with that.

@lcmen
Copy link
Contributor Author

lcmen commented Mar 14, 2020

try import createDOMEvent from '@vue/test-utils/src/create-dom-event. It looks like we do that in other tests. You might need to run yarn build if you made changes in packages. Then yarn test:unit:only. You can also use --watch with that.

Thanks, unfortunately it still doesn't work for me.

I ran: yarn build, then: yarn test:unit:only and now I'm get a following error:

/vue-test-utils/packages/test-utils/src/create-dom-event.js:1
import eventTypes from 'dom-event-types'
       ^^^^^^^^^^

SyntaxError: Unexpected identifier

@lcmen
Copy link
Contributor Author

lcmen commented Mar 19, 2020

Weird. I will pull your branch down this weekend and try it out and see if I can figure out what is going on.

@lmiller1990 by any chance have you been able to look into that?

@lmiller1990
Copy link
Member

@lowski not yet, on the weekend (around 24-48h time).

@lmiller1990
Copy link
Member

lmiller1990 commented Mar 20, 2020

@lowski I see why you can't use the absolute imports now - createDOMEvent is not a public API, so it is not exported from test-utils/index.js, so @vue/test-utils will not worked. I had a look an realize none of our other tests use internal APIs.

I think it's probably okay to use the relative import for now, in this case. The other alternative would be creating the event manually instead of using the createDOMEvent helper, but this will do.

All green, let's merge this up. I'll try and do a release in the next week or so two when I've dealt with the other outstanding PRs.

@lmiller1990 lmiller1990 merged commit b1a532a into vuejs:dev Mar 20, 2020
@lcmen
Copy link
Contributor Author

lcmen commented Mar 20, 2020

@lmiller1990 thank you!

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.

None yet

2 participants