Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Radio button TypeError in CI #8307

Closed
2 of 6 tasks
nwhittaker opened this issue Nov 30, 2023 · 15 comments
Closed
2 of 6 tasks

Radio button TypeError in CI #8307

nwhittaker opened this issue Nov 30, 2023 · 15 comments
Assignees
Labels
4 - verified Issues that have been released and confirmed resolved. ArcGIS Field Apps Issues logged by ArcGIS Field Apps team members. bug Bug reports for broken functionality. Issues should include a reproduction of the bug. calcite-components Issues specific to the @esri/calcite-components package. estimate - 1 Very small fix or change (potentially a single line), doesn't require updates to tests. impact - p2 - want for an upcoming milestone User set priority impact status of p2 - want for an upcoming milestone p - medium Issue is non core or affecting less that 60% of people using the library spike Issues that need quick investigations for time estimations, prioritization, or a quick assessment.

Comments

@nwhittaker
Copy link
Contributor

nwhittaker commented Nov 30, 2023

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/calcite-components
  • @esri/calcite-components-angular
  • @esri/calcite-components-react
  • @esri/calcite-design-tokens
  • @esri/eslint-plugin-calcite-components

Esri team

ArcGIS Field Apps

@nwhittaker nwhittaker added bug Bug reports for broken functionality. Issues should include a reproduction of the bug. 0 - new New issues that need assignment. needs triage Planning workflow - pending design/dev review. labels Nov 30, 2023
@github-actions github-actions bot added calcite-components Issues specific to the @esri/calcite-components package. impact - p2 - want for an upcoming milestone User set priority impact status of p2 - want for an upcoming milestone ArcGIS Field Apps Issues logged by ArcGIS Field Apps team members. labels Nov 30, 2023
@geospatialem geospatialem added p - high Issue should be addressed in the current milestone, impacts component or core functionality estimate - 1 Very small fix or change (potentially a single line), doesn't require updates to tests. labels Dec 11, 2023
@geospatialem geospatialem removed the needs triage Planning workflow - pending design/dev review. label Dec 11, 2023
@geospatialem
Copy link
Member

Potential option for a fix:

otherFocusableRadioButtons.filter(Boolean).forEach((radioButton) => {
      forceUpdate(radioButton);
    });

@nwhittaker
Copy link
Contributor Author

@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
Copy link
Member

driskull commented Jan 3, 2024

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

Something like

if (Build.isBrowser) {
  otherFocusableRadioButtons.filter(Boolean).forEach((radioButton) => {
      forceUpdate(radioButton);
    });
}

@jcfranco jcfranco self-assigned this Jan 4, 2024
@jcfranco jcfranco added 2 - in development Issues that are actively being worked on. and removed 0 - new New issues that need assignment. labels Jan 4, 2024
@nwhittaker
Copy link
Contributor Author

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
Copy link
Member

@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
Copy link
Member

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

@jcfranco
Copy link
Member

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

@jcfranco jcfranco added 1 - assigned Issues that are assigned to a sprint and a team member. need more info Issues that are missing information and/or a clear, actionable description. and removed 2 - in development Issues that are actively being worked on. labels Jan 11, 2024
@nwhittaker
Copy link
Contributor Author

@jcfranco jcfranco added 2 - in development Issues that are actively being worked on. and removed 1 - assigned Issues that are assigned to a sprint and a team member. need more info Issues that are missing information and/or a clear, actionable description. labels Jan 12, 2024
@geospatialem geospatialem removed the p - high Issue should be addressed in the current milestone, impacts component or core functionality label Feb 26, 2024
@jcfranco
Copy link
Member

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

@jcfranco jcfranco added the blocked This issue is blocked by another issue. label Sep 16, 2024
Copy link
Contributor

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

cc @geospatialem,@DitwanP

@github-actions github-actions bot removed the blocked This issue is blocked by another issue. label Nov 26, 2024
@geospatialem geospatialem added the spike Issues that need quick investigations for time estimations, prioritization, or a quick assessment. label Nov 26, 2024
@geospatialem
Copy link
Member

Spike to determine if the above PR mitigates the above.

@geospatialem geospatialem added the needs milestone Planning workflow - pending milestone assignment, has priority and/or estimate. label Nov 26, 2024
@DitwanP DitwanP removed the needs milestone Planning workflow - pending milestone assignment, has priority and/or estimate. label Dec 2, 2024
@DitwanP DitwanP assigned DitwanP and unassigned jcfranco Dec 2, 2024
@DitwanP
Copy link
Contributor

DitwanP commented Dec 11, 2024

Hey @nwhittaker, when you get a chance could you please retest if this is still occurring after Calcite's framework migration (calcite-components version 3.0.0-next.55 is the latest version after migrating as I'm writing this comment).

@nwhittaker
Copy link
Contributor Author

@DitwanP, I retested with Calcite next.115 and no longer see the error.

@geospatialem geospatialem added 4 - verified Issues that have been released and confirmed resolved. and removed 1 - assigned Issues that are assigned to a sprint and a team member. labels Feb 3, 2025
@geospatialem
Copy link
Member

Thanks for testing, @nwhittaker! Closing the above out as mitigated 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4 - verified Issues that have been released and confirmed resolved. ArcGIS Field Apps Issues logged by ArcGIS Field Apps team members. bug Bug reports for broken functionality. Issues should include a reproduction of the bug. calcite-components Issues specific to the @esri/calcite-components package. estimate - 1 Very small fix or change (potentially a single line), doesn't require updates to tests. impact - p2 - want for an upcoming milestone User set priority impact status of p2 - want for an upcoming milestone p - medium Issue is non core or affecting less that 60% of people using the library spike Issues that need quick investigations for time estimations, prioritization, or a quick assessment.
Projects
None yet
Development

No branches or pull requests

5 participants