feat(auto-init): return initialized components

[blikblum]

Make autoInit return initialized components.

Fixes #1329

[googlebot]

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.

[blikblum]

I signed it!

[googlebot]

CLAs look good, thanks!

[Garbee]

What exactly is the reason for wanting this change? Prevent memory leaks? Keep track of changes somehow?

As far as I can tell, even in the issue example, there is no compelling use-case for this to be needed. When the nodes are removed, as long as they have no references left in the JS the related things will be cleaned up.

We fell for a little trap of storing references internally in MDL's auto init system and it was only problematic in the end. I'd rather MCW avoids allowing for that pitfall without tangible gain on having it included.

[blikblum]

What exactly is the reason for wanting this change? Prevent memory leaks? Keep track of changes somehow?

Keep a reference of the initialized MD components in an wrapper component so we can destroy them as advised in Framework Integration guide. It even cites the custom element example:

Note that this approach will also work for custom elements. Use connectedCallback for initialization and disconnectedCallback for destruction.

As a concrete example see this pen. It wraps the Getting Started demo in a custom element. The MD components are initialized with autoInit (using the element as root). Per the integration guide the MD components should be destroyed when the component wrapper is destroyed but currently is not possible because there's not a way to get their references.

The alternative would be initialize and store a reference of each MD component to be destroyed in disconnectedCallback. This is counter productive and error prone. Say that i want to add or remove a new text field or button in the form: i would have to update the template, initialize the component, store a reference and use it to call destroy.

As far as I can tell, even in the issue example, there is no compelling use-case for this to be needed. When the nodes are removed, as long as they have no references left in the JS the related things will be cleaned up.

So is not necessary to call MDComponent.destroy when element is removed? In the pen the form (custom element) can be removed and recreated. It will not cause memory leaks?

[googlebot]

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this State. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

[codecov-io]

Codecov Report

Merging #1333 into master will increase coverage by 0.99%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1333      +/-   ##
==========================================
+ Coverage   98.66%   99.65%   +0.99%     
==========================================
  Files          98       78      -20     
  Lines        4202     3522     -680     
  Branches      534      434     -100     
==========================================
- Hits         4146     3510     -636     
+ Misses         56       12      -44

Impacted Files	Coverage Δ
packages/mdc-auto-init/index.js	100% <100%> (ø)	⬆️
packages/mdc-textfield/helper-text/foundation.js	93.1% <0%> (-1.5%)	⬇️
packages/mdc-checkbox/foundation.js	97.39% <0%> (-0.25%)	⬇️
packages/mdc-icon-toggle/index.js	100% <0%> (ø)	⬆️
packages/mdc-snackbar/index.js	100% <0%> (ø)	⬆️
packages/mdc-checkbox/index.js	100% <0%> (ø)	⬆️
packages/mdc-slider/index.js	100% <0%> (ø)	⬆️
packages/mdc-toolbar/constants.js	100% <0%> (ø)	⬆️
packages/mdc-linear-progress/foundation.js	100% <0%> (ø)	⬆️
packages/mdc-ripple/util.js	100% <0%> (ø)	⬆️
... and 58 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 6f97632...8ba378c. Read the comment docs.

[blikblum]

I confirm

[amsheehan]

@Garbee Can you update the thread on what the pitfalls in MDL were specifically?

[Garbee]

The original MDL component handler stored the initialized elements in an internal cache with no way to remove them. So if you deleted the node in the document, the garbage collector would never destroy it since the init system had a reference. While this directly isn't doing that, the change does open the gateway for that very easy mistake to make to be done by external parties.

Destructing should be optional anyways. When your intent is to delete a node, just delete it and let the garbage collector do the rest. Each component should be engineered so that when it is deleted the garbage collector in browsers can clean up any unneeded things at that point. Also, if you need to destroy a component, you should be capable of calling the destruction on the node itself without needing to keep the references from auto initialization.

[amsheehan]

That's fair enough, but MDC Web isn't storing any references to anything from auto init, so it would have to be a user retaining the reference and not destroying it when intended. You pointed so much out in your comment, but I think we can document and provide functionality in a way that would make it clear that if you store a reference to all of your MDC Web components as a result of using auto init, you'll need to manually destroy them like you would when initializing them and keeping references to them individually.

As far as deleting a dom node, there are some components that listen for resize events on the window (ripple), so it's not as simple as deleting a node and letting the garbage collector do its thing.

[blikblum]

While this directly isn't doing that, the change does open the gateway for that very easy mistake to make to be done by external parties.

It's user responsibility to handle such situations. This is the case for JavaScript in general, not MDC specific. Even without this feature is currently possible to have leaks with the MDC API as in the example below:

import {MDCTextField} from '@material/textfield';

window._mdcComps = [];

const el = document.querySelector('.mdc-text-field')

const textField = new MDCTextField(el);

window._mdcComps.push(textField)

//later
el.parentNode.removeElement(el);

Destructing should be optional anyways

This contradicts the documentation:

"When the wrapper component is destroyed (e.g. it is unbound and detached from the DOM), call mdcComponent.destroy() to clean up the MDC-Web component."

Also, if you need to destroy a component, you should be capable of calling the destruction on the node itself without needing to keep the references from auto initialization.

I can't see in a clean way to do that with autoInit

It would need to call querySelectorAll('[data-mdc-auto-init]') in the root node and walk through the nodes checking for each component name:

function cleanMDCAutoInit(el) { 
  const nodes = el.querySelectorAll('[data-mdc-auto-init]');
  for (let i = 0, node; (node = nodes[i]); i++) {
     if (node.MDCTextField) {
       node.MDCTextField.destroy()
      } else if (node.MDCRipple) {
      node.MDCRipple.destroy()
     } // etc
  }
}

[kfranqueiro]

Sorry this PR lapsed for so long. The changeset looks reasonable to me and I agree with the thoughts expressed by @amsheehan and @blikblum. The CLA was signed and it looks like this can still be merged without conflict, so I'm going to go ahead with it.