Standarise the `fireEvent` method interface #399

khalilovcmd · 2018-07-21T18:31:09Z

Why?

When using triggerEvent to fire a change event on <input> element with type="file", those lines call the buildFileEvent which forwards options with its same structure.

Though, buildFileEvent accepts an array: files = [], the documentation doesn't clarify this difference.

I think it will be more natural not to break the interface here by introducing an array argument.

Sample code

let file = new File(['my-content'],'image.png', {
  type: 'image/png',
});
triggerEvent(selector, 'change', [file]); // will work
triggerEvent(selector, 'change',{ files: file }); // will not propagate the value to the event handler

Questions

Should we start writing a test for this file? It think it is a sensitive one to skip tests on.

Alternatives

We could also live with elaborating on this difference in the API.md, to avoid breaking changes.

cc: kudos to @mattdonnelly for pointing this out, and subsequently saving my day.

rwjblue

Though, buildFileEvent accepts an array: files = [], the documentation doesn't clarify this difference.

buildFileEvent is a private API and should not be used directly, therefore any documentation for this scenario should be in triggerEvent. Can you add additional examples to those inline docs to show how to trigger change on a file input?

Should we start writing a test for this file? I think it is a sensitive one to skip tests on.

Every test we have around eventing is testing fire-event, it is covered fairly well in general but there may be specific event types that are not. We should enumerate the event types and ensure that we have at least one test for each...

rwjblue · 2018-07-23T15:30:42Z

addon-test-support/@ember/test-helpers/dom/fire-event.js

  let event = buildBasicEvent(type);
+  let files = options.files || [];


As written, this is a breaking change. As mentioned in the PR description, anyone using triggerEvent for this today must pass an array, if we are going to change this to { files: someArray } as you suggest here, we definitely need to detect if options is already an array...

if we are going to change this to { files: someArray } as you suggest here, we definitely need to detect if options is already an array...

@rwjblue that makes sense. My first thought is to disambiguate this in the documentation itself, but then as a novice user of this add on, I would have expected that the interface to accept { files: someArray }, hence why I felt it might be a more natural (but breaking change) step to standarize it.

khalilovcmd · 2018-07-23T15:40:22Z

Thank you @rwjblue for taking a look.

To summarise, I could take these two actions, as you suggested:

Can you add additional examples to those inline docs to show how to trigger change on a file input?

I will add more examples to the documentation of the triggerEvent to elaborate on how to trigger a change event for a file.
Detect if options is already an array.

Does this sound like a good way to move forward?

rwjblue · 2018-07-23T15:44:53Z

Yep, sounds good to me!

khalilovcmd · 2018-07-25T08:40:33Z

@rwjblue does it make sense the way it looks now?

When using `triggerEvent` to fire a `change` event on `<input> element with type="file"`, [those lines](https://github.com/emberjs/ember-test-helpers/blob/8a3614657064efaf13dfe667fe4cfdc14f15f509/addon-test-support/@ember/test-helpers/dom/fire-event.js#L57-L58) call the `buildFileEvent` which forwards `options` with its same structure. Though, `buildFileEvent` accepts an array: `files = []`, the documentation doesn't clarify this difference. I think it will be more natural not to break the interface here by introducing an array argument. Standarise the `fireEvent` method interface When using `triggerEvent` to fire a `change` event on `<input> element with type="file"`, [those lines](https://github.com/emberjs/ember-test-helpers/blob/8a3614657064efaf13dfe667fe4cfdc14f15f509/addon-test-support/@ember/test-helpers/dom/fire-event.js#L57-L58) call the `buildFileEvent` which forwards `options` with its same structure. Though, `buildFileEvent` accepts an array: `files = []`, the documentation doesn't clarify this difference. I think it will be more natural not to break the interface here by introducing an array argument. Standarise the `fireEvent` method interface When using `triggerEvent` to fire a `change` event on `<input> element with type="file"`, [those lines](https://github.com/emberjs/ember-test-helpers/blob/8a3614657064efaf13dfe667fe4cfdc14f15f509/addon-test-support/@ember/test-helpers/dom/fire-event.js#L57-L58) call the `buildFileEvent` which forwards `options` with its same structure. Though, `buildFileEvent` accepts an array: `files = []`, the documentation doesn't clarify this difference. I think it will be more natural not to break the interface here by introducing an array argument. Apply PR feedback 1. Added an example to the documentation of the `triggerEvent` to elaborate on how to trigger a change event for a file. 2. Added logic for detecting if options is already an array. This commit allows us to avoid introducing breaking changes, and maintain an expectation of providing `files` array in the `options` object for the `triggerEvent` method. Standarise the `fireEvent` method interface When using `triggerEvent` to fire a `change` event on `<input> element with type="file"`, [those lines](https://github.com/emberjs/ember-test-helpers/blob/8a3614657064efaf13dfe667fe4cfdc14f15f509/addon-test-support/@ember/test-helpers/dom/fire-event.js#L57-L58) call the `buildFileEvent` which forwards `options` with its same structure. Though, `buildFileEvent` accepts an array: `files = []`, the documentation doesn't clarify this difference. I think it will be more natural not to break the interface here by introducing an array argument. Apply PR feedback 1. Added an example to the documentation of the `triggerEvent` to elaborate on how to trigger a change event for a file. 2. Added logic for detecting if options is already an array. This commit allows us to avoid introducing breaking changes, and maintain an expectation of providing `files` array in the `options` object for the `triggerEvent` method. Fix lint offense Fix lint offense Standarise the `fireEvent` method interface When using `triggerEvent` to fire a `change` event on `<input> element with type="file"`, [those lines](https://github.com/emberjs/ember-test-helpers/blob/8a3614657064efaf13dfe667fe4cfdc14f15f509/addon-test-support/@ember/test-helpers/dom/fire-event.js#L57-L58) call the `buildFileEvent` which forwards `options` with its same structure. Though, `buildFileEvent` accepts an array: `files = []`, the documentation doesn't clarify this difference. I think it will be more natural not to break the interface here by introducing an array argument. Apply PR feedback 1. Added an example to the documentation of the `triggerEvent` to elaborate on how to trigger a change event for a file. 2. Added logic for detecting if options is already an array. This commit allows us to avoid introducing breaking changes, and maintain an expectation of providing `files` array in the `options` object for the `triggerEvent` method. Fix lint offense Standarise the `fireEvent` method interface When using `triggerEvent` to fire a `change` event on `<input> element with type="file"`, [those lines](https://github.com/emberjs/ember-test-helpers/blob/8a3614657064efaf13dfe667fe4cfdc14f15f509/addon-test-support/@ember/test-helpers/dom/fire-event.js#L57-L58) call the `buildFileEvent` which forwards `options` with its same structure. Though, `buildFileEvent` accepts an array: `files = []`, the documentation doesn't clarify this difference. I think it will be more natural not to break the interface here by introducing an array argument. Apply PR feedback 1. Added an example to the documentation of the `triggerEvent` to elaborate on how to trigger a change event for a file. 2. Added logic for detecting if options is already an array. This commit allows us to avoid introducing breaking changes, and maintain an expectation of providing `files` array in the `options` object for the `triggerEvent` method. Fix lint offense Fix lint offense Standarise the `fireEvent` method interface When using `triggerEvent` to fire a `change` event on `<input> element with type="file"`, [those lines](https://github.com/emberjs/ember-test-helpers/blob/8a3614657064efaf13dfe667fe4cfdc14f15f509/addon-test-support/@ember/test-helpers/dom/fire-event.js#L57-L58) call the `buildFileEvent` which forwards `options` with its same structure. Though, `buildFileEvent` accepts an array: `files = []`, the documentation doesn't clarify this difference. I think it will be more natural not to break the interface here by introducing an array argument. Apply PR feedback 1. Added an example to the documentation of the `triggerEvent` to elaborate on how to trigger a change event for a file. 2. Added logic for detecting if options is already an array. This commit allows us to avoid introducing breaking changes, and maintain an expectation of providing `files` array in the `options` object for the `triggerEvent` method. Fix lint offense Fix lint offense Fix identation in the docs Fix lint issues

rwjblue · 2018-07-27T14:49:23Z

addon-test-support/@ember/test-helpers/dom/fire-event.js

  let event = buildBasicEvent(type);
+  let files = Array.isArray(options) ? options : options.files || [];


Should we issue a deprecation or something when we do this coercion?

I'm just thinking that over time we probably only want to support one form...

rwjblue · 2018-07-27T14:52:04Z

addon-test-support/@ember/test-helpers/dom/trigger-event.js

+ *
+ *     // passing an array of files
+ *     let file = new File(['text file'], 'text.txt', { type: 'text/plain' });
+ *     await triggerEvent('input:file', 'change', [file]);


I think ultimately, we should only have one format here (probably the { files: [] } one?)...

rwjblue · 2018-08-06T18:59:31Z

@khalilovcmd - Had a chance to review those questions?

rwjblue · 2018-08-07T22:28:15Z

Will need to address merge conflicts from docs changes made in #340

rwjblue · 2018-09-27T14:34:05Z

@khalilovcmd - I'd still love to land this, any chance you mind have time to rebase and clean up soon?

rwjblue requested changes Jul 23, 2018

View reviewed changes

khalilovcmd force-pushed the tarek/standarise-the-fireEvent-method-interface branch 3 times, most recently from 0efac39 to 9e6a980 Compare July 25, 2018 08:40

khalilovcmd force-pushed the tarek/standarise-the-fireEvent-method-interface branch from 9e6a980 to 5ddfe54 Compare July 25, 2018 13:45

rwjblue reviewed Jul 27, 2018

View reviewed changes

mattdonnelly mentioned this pull request Oct 15, 2018

Standardize params for calling triggerEvent on file input #428

Merged

rwjblue closed this in #428 Oct 16, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Standarise the `fireEvent` method interface #399

Standarise the `fireEvent` method interface #399

khalilovcmd commented Jul 21, 2018 •

edited

rwjblue left a comment

rwjblue Jul 23, 2018

khalilovcmd Jul 23, 2018

khalilovcmd commented Jul 23, 2018

rwjblue commented Jul 23, 2018

khalilovcmd commented Jul 25, 2018

rwjblue Jul 27, 2018

rwjblue Jul 27, 2018

rwjblue commented Aug 6, 2018

rwjblue commented Aug 7, 2018

rwjblue commented Sep 27, 2018

		let event = buildBasicEvent(type);
		let files = options.files \|\| [];

		let event = buildBasicEvent(type);
		let files = Array.isArray(options) ? options : options.files \|\| [];

Standarise the fireEvent method interface #399

Standarise the fireEvent method interface #399

Conversation

khalilovcmd commented Jul 21, 2018 • edited

Why?

Sample code

Questions

Alternatives

rwjblue left a comment

Choose a reason for hiding this comment

rwjblue Jul 23, 2018

Choose a reason for hiding this comment

khalilovcmd Jul 23, 2018

Choose a reason for hiding this comment

khalilovcmd commented Jul 23, 2018

rwjblue commented Jul 23, 2018

khalilovcmd commented Jul 25, 2018

rwjblue Jul 27, 2018

Choose a reason for hiding this comment

rwjblue Jul 27, 2018

Choose a reason for hiding this comment

rwjblue commented Aug 6, 2018

rwjblue commented Aug 7, 2018

rwjblue commented Sep 27, 2018

Standarise the `fireEvent` method interface #399

Standarise the `fireEvent` method interface #399

khalilovcmd commented Jul 21, 2018 •

edited