Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The Bug
The current implementation of Icon.js was not guarding against
connectedCallback
being called multiple times.See details for when this may happen.
Details
The most common scenario in our UXDS would be when we're adding the
<cod-icon/>
element to another slotted custom element. This causes the<cod-icon/>
component to be connected twice: once when attached to the slot, then again whenonslotchange
event handler attaches it to the shadowDOM of the parent component. E.g.NOTE: In the below example, the component is actually connected three times because both
cod-card
andcod-card-icon-container
are slotted elements.This results in the following:
Proposed Fix (This PR)
Add a guard in
connectedCallback
to check if the icon span is already attached to the shadowDom and return early if so.Alternatives Considered
We could attach the shadow root as part of the
connectedCallback
method which would recreate a shadowDOM each time the component is attached to the DOM. This is a reasonable alternative but the performance implications are unclear andconnectedCallback
can be called many times.We could remove the icon element from the shadowDOM on
disconnectedCallback
lifecycle event. This type of lifecycle management might be appropriate if we were managing some state that needed to be reset, but since the icon elememnt does not change between lifecycle events, it's probably best to just let it live between events.Testing