Esri / calcite-design-system

A monorepo containing the packages for Esri's Calcite Design System
https://developers.arcgis.com/calcite-design-system/
Other
295 stars 76 forks source link

Radio button `TypeError` in CI #8307

Open nwhittaker opened 12 months ago

nwhittaker commented 12 months ago

Check existing issues

Actual Behavior

A <calcite-radio-button-group> within a storybook StoryFn sometimes throws a TypeError: Cannot read properties of undefined (reading '$hostElement$') error when running the storybook tests in CI.

Expected Behavior

This error is not thrown.

Reproduction Sample

https://developers.arcgis.com/calcite-design-system/components/radio-button-group/

Reproduction Steps

I don't have a reduced test case since the issue is intermittent and likely stems from race conditions.

In a storybook StoryFn, I have this snippet within a custom component's details slot:

<calcite-radio-button-group name="details" slot="details">
  <calcite-label layout="inline">
    <calcite-radio-button value="one"></calcite-radio-button> One
  </calcite-label>
  <calcite-label layout="inline">
    <calcite-radio-button value="two"></calcite-radio-button> Two
  </calcite-label>
</calcite-radio-button-group>

The issue seems to have cropped up after recently upgrading to both Calcite 1.11 and Storybook 7.5.

Reproduction Version

1.11.0

Relevant Info

[TEST]     error: TypeError: Cannot read properties of undefined (reading '$hostElement$')
[TEST]     TypeError: Cannot read properties of undefined (reading '$hostElement$')
[TEST] 
[TEST]       at forceUpdate (packages/field-apps-custom-elements/http:/127.0.0.1:6006/vendors-node_modules_esri_calcite-components_dist_components_index_js-node_modules_esri_calci-246f15.iframe.bundle.js:2683:37)
[TEST]       at packages/field-apps-custom-elements/http:/127.0.0.1:6006/vendors-node_modules_esri_calcite-components_dist_esm_calcite-radio-button_entry_js.iframe.bundle.js:303:60
[TEST]           at Array.forEach (<anonymous>)
[TEST]       at RadioButton.updateTabIndexOfOtherRadioButtonsInGroup (packages/field-apps-custom-elements/http:/127.0.0.1:6006/vendors-node_modules_esri_calcite-components_dist_esm_calcite-radio-button_entry_js.iframe.bundle.js:302:32)
[TEST]       at RadioButton.connectedCallback (packages/field-apps-custom-elements/http:/127.0.0.1:6006/vendors-node_modules_esri_calcite-components_dist_esm_calcite-radio-button_entry_js.iframe.bundle.js:343:10)
[TEST]       at safeCall (packages/field-apps-custom-elements/http:/127.0.0.1:6006/vendors-node_modules_esri_calcite-components_dist_components_index_js-node_modules_esri_calci-246f15.iframe.bundle.js:2703:36)
[TEST]       at fireConnectedCallback (packages/field-apps-custom-elements/http:/127.0.0.1:6006/vendors-node_modules_esri_calcite-components_dist_components_index_js-node_modules_esri_calci-246f15.iframe.bundle.js:2960:9)
[TEST]       at initializeComponent (packages/field-apps-custom-elements/http:/127.0.0.1:6006/vendors-node_modules_esri_calcite-components_dist_components_index_js-node_modules_esri_calci-246f15.iframe.bundle.js:2929:13)
[TEST]       at connectedCallback (packages/field-apps-custom-elements/http:/127.0.0.1:6006/vendors-node_modules_esri_calcite-components_dist_components_index_js-node_modules_esri_calci-246f15.iframe.bundle.js:3012:17)
[TEST]       at packages/field-apps-custom-elements/http:/127.0.0.1:6006/vendors-node_modules_esri_calcite-components_dist_components_index_js-node_modules_esri_calci-246f15.iframe.bundle.js:3120:39, undefined
[TEST]       at Object.playFunctionThrewException (packages/field-apps-custom-elements/<anonymous>:343:16)

Looking at the stacktrace, I suspect updateTabIndexOfOtherRadioButtonsInGroup() is calling forceUpdate() on the other <calcite-radio-button> elements in the group after they're inserted into the DOM but before they've hydrated.

Regression?

No response

Priority impact

p2 - want for current milestone

Impact

This issue impacts our PR process where we have to manually re-run these tests until they pass.

Calcite package

Esri team

ArcGIS Field Apps

geospatialem commented 11 months ago

Potential option for a fix:

otherFocusableRadioButtons.filter(Boolean).forEach((radioButton) => {
      forceUpdate(radioButton);
    });
nwhittaker commented 11 months ago

@geospatialem, I don't think that snippet is adequate since the Boolean filter will still pass for pre-hydrated radio buttons, right?

I updated the issue description to clarify when I think the error is occurring:

…is calling forceUpdate() on the other <calcite-radio-button> elements in the group after they're inserted into the DOM but before they've hydrated.

driskull commented 11 months ago

Maybe we only call forceUpdate if the build is a browser?

Something like

if (Build.isBrowser) {
  otherFocusableRadioButtons.filter(Boolean).forEach((radioButton) => {
      forceUpdate(radioButton);
    });
}
nwhittaker commented 10 months ago

Maybe we only call forceUpdate if the build is a browser?

  1. Our CI tests run in a headless browser -- I'm not sure they'd fail the Build.isBrowser check(?)
  2. We'd probably still want to call forceUpdate at other points in the component's lifecycle/watch changes. It's really only calling it during connectedCallback() that has the problem.
  3. To reiterate, that Boolean filter isn't doing anything. It'd need to reject non-hydrated radio buttons to have the, I believe, intended effect.
jcfranco commented 10 months ago

@nwhittaker Are you still on version 1.11.0? v2 bumped the Stencil version, which fixed some component lifecycle issues we noticed.

In any case, I think checking for Build.isBrowser might not help since forceUpdate already does under the hood.

jcfranco commented 10 months ago

@nwhittaker I'll reach out to you directly for more info as I haven't been able to repro. 😞

jcfranco commented 10 months ago

Quick update: reached out to @nwhittaker and he'll be testing with v2 to confirm whether the issue is still present.

nwhittaker commented 10 months ago

I tested with v2.1.0 and am seeing the same error:

Error run: https://github.com/ArcGIS/acadia-web/actions/runs/7492111875/job/20394912129?pr=4492#step:5:661 Problematic test case: https://github.com/ArcGIS/acadia-web/blob/d1c5bd512405e67d1c04448e9ae906eb8f24d2ba/packages/field-apps-custom-elements/src/components/radio-button-group/radio-button-group.stories.ts#L79

geospatialem commented 9 months ago

Reallocating to target in April as the test mentioned is currently being skipped, and additional R&D is needed to determine a fix.

jcfranco commented 2 months ago

This may no longer be an issue once #10310 lands.

github-actions[bot] commented 2 days ago

Issue #10310 has been closed, this issue is ready for re-evaluation.

cc @geospatialem,@DitwanP

geospatialem commented 2 days ago

Spike to determine if the above PR mitigates the above.