feat(auto-init): Fire event on mdcAutoInit complete #1012

yshterev · 2017-07-25T23:01:34Z

@yeelan0319 The dialog component is extending MDCComponent so it has direct access to emit method. I am not sure how to access the emit method properly in mdcAutoInit function. I tried to import MDCComponent, but I guess it is not good.

Also I am not sure what the test should be.
Any help appreciated.

googlebot · 2017-07-25T23:01:37Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

yshterev · 2017-07-25T23:03:24Z

I signed it!

googlebot · 2017-07-25T23:03:25Z

CLAs look good, thanks!

codecov-io · 2017-07-25T23:27:06Z

Codecov Report

Merging #1012 into master will decrease coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master   #1012      +/-   ##
=========================================
- Coverage   99.93%   99.9%   -0.03%     
=========================================
  Files          68      69       +1     
  Lines        3276    3314      +38     
  Branches      403     409       +6     
=========================================
+ Hits         3274    3311      +37     
- Misses          2       3       +1

Impacted Files	Coverage Δ
packages/mdc-auto-init/index.js	`100% <100%> (ø)`	⬆️
packages/mdc-select/foundation.js	`100% <0%> (ø)`	⬆️
packages/mdc-dialog/index.js	`100% <0%> (ø)`	⬆️
packages/mdc-form-field/constants.js	`100% <0%> (ø)`	⬆️
packages/mdc-dialog/foundation.js	`100% <0%> (ø)`	⬆️
packages/mdc-snackbar/foundation.js	`100% <0%> (ø)`	⬆️
packages/mdc-form-field/foundation.js	`100% <0%> (ø)`	⬆️
packages/mdc-radio/index.js	`97.67% <0%> (ø)`	⬆️
packages/mdc-form-field/index.js	`100% <0%> (ø)`	⬆️
packages/mdc-textfield/index.js	`100% <0%> (ø)`	⬆️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update defa7ac...5691dee. Read the comment docs.

yeelan0319 · 2017-07-26T15:42:11Z

@yshterev Thanks for filing the PR.

So here in the auto-init you don't really need to import MDCComponent - all the components themselves are instances of an derived MDCComponent and have this emit method.

For firing event
Take a look at mdc-auto-init index.js line 45 where an component instance is created here.
You can add component.emit('MDCAutoInit:End', {}, true); after line 51. It will make the component emit an MDCAutoInit:End event after it inits.
You can find a detailed explanation of the argument explanation in mdc-base document.

Test firing event
Take a look at unit test for mdc-tab-bar line 234. It need some modification but basically you can see a good reference how to test whether a event if fired.

Listen to fired event (FYI)
I also created a pen illustrate how the custom event could be listened.

Feel free to ping me back for further question and thanks again for being on it!

yshterev · 2017-07-27T11:10:06Z

@yeelan0319 The FakeComponent in mdc-auto-init test was too fake. It did not have emit method, so I decided to use add the real mdc-base component. I think this way the test will be more accurate.

I am not sure my test logic is very good. It relies on the first argument passed to the handler func being the event object. Please review.

yeelan0319 · 2017-08-02T20:28:20Z

packages/mdc-auto-init/index.js

    Object.defineProperty(node, ctorName, {
      value: component,
      writable: false,
      enumerable: false,
      configurable: true,
    });
  }
+
+  if (component !== null) {


There might be multiple components initialize with mdc-auto-init method and my original thought is each of them should have their own init:end event.

If you are interested in just having one custom event about the whole auto-init end, maybe we shouldn't do that from a component level.

Probably I got this wrong.

If each component emits its own init:end event when done, how would you know when all of components are initialzied? Or maybe we can use combination of both? Each component can emit own event when initialized, like ComponentNameInit:End and then 'MDCAutoInit:End' when all of them are initialized?

If I understand correctly the original issue was regarding having a way to tell when all components initialize, so I agree that should be the main goal of this PR.

Having one event fire per component init seems potentially excessive and is perhaps a separate bridge we should cross if a convincing use case arises?

Also, I'm not sure whether it makes sense to have the "all initialized" event emit from a particular component, since which component emits it could change just based on how an app changes during development. I'm open to thoughts/arguments otherwise here though.

@kfranqueiro When I submitted the issue I was not sure. The discussion in the issue makes it clear it is about having a way to tell when all components initialize, so I changed its name to be more clear.

I guess using the component to trigger the event was suggested by @yeelan0319 to keep it DRY and reuse the emit method.

I also think the PR should resolve the issue, meaning "all initialized" only until other use case arises.

@yshterev I looked at the comments on the issue and discussion here. I think it was slightly different though when we discuss it internally as we thought it would be done at component level.
So as now we understand the landscape of the question better, I think it should be proper to dispatch a custom event directly instead of using emit method from a single component. Could you update the PR accordingly? Thanks!

yeelan0319 · 2017-08-02T20:44:40Z

test/unit/mdc-auto-init/mdc-auto-init.test.js

+    return this.root_;
+  }
+
+  getDefaultFoundation() {


Why we are adding Foundation method here? It would inherent from MDCComponent so you don't need to reimplement it.

Some tests don't pass. The error is Error: Subclasses must override getDefaultFoundation to return a properly configured foundation class

I think once we directly implemented emit function, we no longer need the fake element here to inherit from MDCComponent. We can safely change it back to what it was and simply adding two new test cases.

yeelan0319 · 2017-08-02T20:45:31Z

test/unit/mdc-auto-init/mdc-auto-init.test.js

+  mdcAutoInit(root);
+
+  td.verify(handler(td.matchers.isA(Object)));
+});


Test looks good!

yeelan0319 · 2017-08-02T20:47:34Z

packages/mdc-auto-init/README.md

+### Events
+
+#### MDCAutoInit:End
+Triggered when initialization of all components is complete.


If that's the case, we probably shouldn't rely on component to emit event.

See my comment below

lynnmercier · 2017-08-18T18:09:15Z

Hey @yshterev, is there anything unclear about the changes requested here?

yshterev · 2017-08-21T11:23:11Z

@lynnjepsen Sorry for the delay. PR updated.

yshterev · 2017-08-23T09:57:15Z

What should I do to pass the codecov test? Maybe rebase from master?

yeelan0319

@yshterev Thanks for updating the PR. The codecov failed because line 37/38 in mdc-auto-init.js is not being covered, thus make the code coverage drop below the requirement. Please take a look at line 157 in test/unit/mdc-base/components.test.js to see how emit is tested.

yeelan0319 · 2017-08-23T19:13:25Z

test/unit/mdc-auto-init/mdc-auto-init.test.js

+    return this.root_;
+  }
+
+  getDefaultFoundation() {


I think once we directly implemented emit function, we no longer need the fake element here to inherit from MDCComponent. We can safely change it back to what it was and simply adding two new test cases.

yeelan0319 · 2017-08-29T15:52:36Z

@yshterev Any updates to the changes or let me know if you have more questions!

yshterev · 2017-08-30T11:40:06Z

FakeComponent in test reverted. 2 new tests added for the emit function.

yeelan0319 · 2017-09-01T19:55:04Z

packages/mdc-auto-init/index.js

+ * @param {!Object} evtData
+ * @param {boolean=} shouldBubble
+ */
+export function emit(element, evtType, evtData, shouldBubble = false) {


IMHO, I might not consider making emit a private helper function of mdc-auto-init instead of public. So far, I cannot see a strong argument why this method need to be public.

If testing is your biggest concern, how about cover both branch using the following methods?
The logic behind this is, we don't need explicitly export emit to test it but we mock the run-time environment (as you did in testing emit directly) and see if mdcAutoInit behave uniformly in both case.

test('#emit event from MDCComponent on init complete', () => { const root = setupTest(); const handler = td.func('init event handler'); let evt = null; td.when(handler(td.matchers.isA(Object))).thenDo((evt_) => { evt = evt_; }); const type = 'MDCAutoInit:End'; document.addEventListener(type, handler); mdcAutoInit(root); assert.isOk(evt !== null); assert.equal(evt.type, type); }); test('#emit event from MDCComponent on init complete - custom events not supported', () => { const root = setupTest(); const handler = td.func('init event handler'); let evt = null; td.when(handler(td.matchers.isA(Object))).thenDo((evt_) => { evt = evt_; }); const type = 'MDCAutoInit:End'; document.addEventListener(type, handler); const {CustomEvent} = window; window.CustomEvent = undefined; try { mdcAutoInit(root); } finally { window.CustomEvent = CustomEvent; } assert.isOk(evt !== null); assert.equal(evt.type, type); });

Let me know if this makes sense or your consideration for making emit public. Thanks!

Sure. Your proposal is better. I exported emit just because of the test. Then the coverage did not pass again and I saw I was missing another test.
Do you think I should remove the element param from the emit function and just use document? I don't think element param makes sense anymore because tests always attach the listener to document.

@yshterev Sorry for the tardiness. Yes. I think that is totally a valid suggestion, right now we are only emitting event at document level, which means the element is not really being used. Feel free to proceed with this idea and let me know when it is ready to review.

yshterev · 2017-09-10T10:15:44Z

@yeelan0319 Updated

yeelan0319

@yshterev Thanks for the update. I am still a bit worried that we make emit a public function. I left some suggestions and see if you could agree with it.

yeelan0319 · 2017-09-10T14:24:49Z

packages/mdc-auto-init/index.js

+ * @param {!Object} evtData
+ * @param {boolean=} shouldBubble
+ */
+export function emit(evtType, evtData, shouldBubble = false) {


Could we consider making this a private method?
My concern is there might not be a use case where this function will be called by external user (now).

We could do function emit_ to identify this is a private method and remove export. I also commented about revoke and test below.

What do you think?

yeelan0319 · 2017-09-10T14:27:48Z

packages/mdc-auto-init/index.js

@@ -49,6 +70,8 @@ export default function mdcAutoInit(root = document, warn = CONSOLE_WARN) {
      configurable: true,
    });
  }
+
+  emit('MDCAutoInit:End', {});


_emit('MDCAutoInit:End', {}); instead.

yeelan0319 · 2017-09-10T14:29:01Z

test/unit/mdc-auto-init/mdc-auto-init.test.js

@@ -18,6 +18,7 @@ import bel from 'bel';
 import td from 'testdouble';
 import {assert} from 'chai';
 import mdcAutoInit from '../../../packages/mdc-auto-init';
+import {emit} from '../../../packages/mdc-auto-init';


We can proxy over mdcAutoInit to test private method emit is called, so we could remove this line.

yeelan0319 · 2017-09-10T14:31:22Z

test/unit/mdc-auto-init/mdc-auto-init.test.js

@@ -95,3 +96,47 @@ test('#register warns when registered key is being overridden', () => {

  td.verify(warn(contains('(mdc-auto-init) Overriding registration')));
 });
+
+test('#emit dispatches a custom event with supplied data', () => {


Consider rewording '#emit dispatches a custom event with supplied data' > '#mdcAutoInit dispatches a MDCAutoInit:End event when initialization of all components is complete'

yeelan0319 · 2017-09-10T14:31:51Z

test/unit/mdc-auto-init/mdc-auto-init.test.js

+    evt = evt_;
+  });
+
+  const data = {evtData: true};


I think we could remove this line as the event doesn't come with data

yeelan0319 · 2017-09-10T14:32:36Z

test/unit/mdc-auto-init/mdc-auto-init.test.js

+
+  assert.isOk(evt !== null);
+  assert.equal(evt.type, type);
+  assert.deepEqual(evt.detail, data);


If we remove data above, also here.

yeelan0319 · 2017-09-10T14:33:01Z

test/unit/mdc-auto-init/mdc-auto-init.test.js

+  assert.deepEqual(evt.detail, data);
+});
+
+test('#emit dispatches an event with supplied data - custom events not supported', () => {


Same thing here. Consider rewording '#mdcAutoInit dispatches a MDCAutoInit:End event when initialization of all components is complete - custom events not supported'?

yeelan0319 · 2017-09-10T14:33:23Z

test/unit/mdc-auto-init/mdc-auto-init.test.js

+    evt = evt_;
+  });
+
+  const data = {evtData: true};


yeelan0319 · 2017-09-10T14:33:33Z

test/unit/mdc-auto-init/mdc-auto-init.test.js

+  const {CustomEvent} = window;
+  window.CustomEvent = undefined;
+  try {
+    emit(type, data);


yeelan0319 · 2017-09-10T14:33:39Z

test/unit/mdc-auto-init/mdc-auto-init.test.js

+
+  assert.isOk(evt !== null);
+  assert.equal(evt.type, type);
+  assert.deepEqual(evt.detail, data);


yshterev · 2017-09-10T15:53:33Z

@yeelan0319 Sorry .. I was sleepy I guess. Updated.

yeelan0319

Thanks @yshterev for keeping contributing on it, everything looks great 👍

yshterev · 2017-09-11T21:43:58Z

@yeelan0319 My pleasure 👍

feat(auto-init): Fire event on mdcAutoInit complete

e4cc438

yshterev added 2 commits July 27, 2017 13:39

feat(auto-init): Fire event on mdcAutoInit complete

751588c

feat(auto-init): Update docs

1a3ea74

yeelan0319 suggested changes Aug 2, 2017

View reviewed changes

lynnmercier assigned yeelan0319 Aug 10, 2017

feat(auto-init): Update

92b9d7e

yeelan0319 suggested changes Aug 23, 2017

View reviewed changes

yshterev added 2 commits August 30, 2017 14:06

feat(auto-init): Update mdcAutoInit tests

7b821a7

feat(auto-init): Update mdcAutoInit tests - custom events not supported

6f433e6

yeelan0319 suggested changes Sep 1, 2017

View reviewed changes

feat(auto-init): Update mdcAutoInit tests and emit func

424e1ed

yeelan0319 suggested changes Sep 10, 2017

View reviewed changes

feat(auto-init): Update mdcAutoInit tests and emit func

e281732

yeelan0319 added 2 commits September 11, 2017 15:42

fix(auto-init): Remove doc string from private method

4ed8dff

fix(auto-init): Modify test name

5691dee

yeelan0319 approved these changes Sep 11, 2017

View reviewed changes

yeelan0319 merged commit 08b5a32 into material-components:master Sep 11, 2017

feat(auto-init): Fire event on mdcAutoInit complete #1012

feat(auto-init): Fire event on mdcAutoInit complete #1012

Conversation

yshterev commented Jul 25, 2017

googlebot commented Jul 25, 2017

yshterev commented Jul 25, 2017

googlebot commented Jul 25, 2017

codecov-io commented Jul 25, 2017 • edited Loading

Codecov Report

yeelan0319 commented Jul 26, 2017

yshterev commented Jul 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yeelan0319 Aug 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lynnmercier commented Aug 18, 2017

yshterev commented Aug 21, 2017

yshterev commented Aug 23, 2017

yeelan0319 left a comment • edited Loading

Choose a reason for hiding this comment

yeelan0319 Aug 23, 2017 • edited Loading

Choose a reason for hiding this comment

yeelan0319 commented Aug 29, 2017

yshterev commented Aug 30, 2017

Choose a reason for hiding this comment

yshterev Sep 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yshterev commented Sep 10, 2017

yeelan0319 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yshterev commented Sep 10, 2017

yeelan0319 left a comment

Choose a reason for hiding this comment

yshterev commented Sep 11, 2017

codecov-io commented Jul 25, 2017 •

edited

Loading

yeelan0319 Aug 23, 2017 •

edited

Loading

yeelan0319 left a comment •

edited

Loading

yeelan0319 Aug 23, 2017 •

edited

Loading

yshterev Sep 1, 2017 •

edited

Loading