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

Fix computation error on disclosure utility #456

Merged
merged 2 commits into from
Jul 18, 2022

Conversation

alex-ju
Copy link
Member

@alex-ju alex-ju commented Jul 18, 2022

πŸ“Œ Summary

Fix computation error on disclosure utility component

πŸ› οΈ Detailed description

Ember, possibly on certain versions (3.2x), doesn't support setting and getting a @tracked property in the same computation. To avoid this type of errors, we're using an explicit conditional statement to toggle the isOpen property, so we get the property in a computation and set it in a different one.

Sample error:

Error: You attempted to update `isOpen` on `HdsDisclosureComponent`, but it had already been used previously in the same computation.  Attempting to update a value after using it in a computation can cause logical errors, infinite revalidation bugs, and performance issues, and is not supported.

πŸ”— External links

Issue raised in Slack


πŸ‘€ How to review

πŸ‘‰ Review by files changed

Reviewer's checklist:

  • +1 Percy if applicable
  • Confirm that PR has a changelog update via Changesets if needed

πŸ’¬ Please consider using conventional comments when reviewing this PR.

@vercel
Copy link

vercel bot commented Jul 18, 2022

The latest updates on your projects. Learn more about Vercel for Git β†—οΈŽ

Name Status Preview Updated
hds-components βœ… Ready (Inspect) Visit Preview Jul 18, 2022 at 9:44AM (UTC)
hds-flight-website βœ… Ready (Inspect) Visit Preview Jul 18, 2022 at 9:44AM (UTC)

@changeset-bot
Copy link

changeset-bot bot commented Jul 18, 2022

πŸ¦‹ Changeset detected

Latest commit: d0634a6

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@hashicorp/design-system-components Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@alex-ju alex-ju requested a review from didoo July 18, 2022 08:49
Copy link
Contributor

@didoo didoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all good, just please add a comment in the code explaining why it's necessary to have that strange "logic"

Ember, possibly on certain versions, doesn't support setting and getting a `@tracked` property in the same computation. To avoid this type of errors, we're using an explicit conditional statement to toggle the `isOpen` property.
@alex-ju alex-ju force-pushed the alex-ju/fix-computation-error-on-disclosure branch from d037db1 to d0634a6 Compare July 18, 2022 09:42
@vercel vercel bot temporarily deployed to Preview – hds-flight-website July 18, 2022 09:44 Inactive
@vercel vercel bot temporarily deployed to Preview – hds-components July 18, 2022 09:44 Inactive
@alex-ju alex-ju merged commit a488ea0 into main Jul 18, 2022
@alex-ju alex-ju deleted the alex-ju/fix-computation-error-on-disclosure branch July 18, 2022 10:18
@hashibot-hds hashibot-hds mentioned this pull request Jul 18, 2022
alex-ju added a commit that referenced this pull request Jul 18, 2022
In #456 I wrongly identified the issue. After managing to replicate the issue locally and testing with hashicorp/cloud-ui#3098 it turns out the racing happens between all the action functions (onFocusOut, onKeyUp) triggering the same `notifyPropertyChange` via `close` in a short amount of time (even shorter in automated tests).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants