components icon indicating copy to clipboard operation
components copied to clipboard

chore: Expose more component configuration in DOM metadata property

Open connorlanigan opened this issue 1 year ago • 4 comments

Description

This change extends the __awsuiMetadata__ property of 28 components to include information about their configured properties.

It builds on https://github.com/cloudscape-design/components/pull/1391, build failures are expected until the other PR is merged.

Related links, issue #, if available: n/a

How has this been tested?

Manual testing in the dev pages

Review checklist

The following items are to be evaluated by the author(s) and the reviewer(s).

Correctness

  • Changes include appropriate documentation updates.
  • Changes are backward-compatible if not indicated, see CONTRIBUTING.md.
  • Changes do not include unsupported browser features, see CONTRIBUTING.md.
  • Changes were manually tested for accessibility, see accessibility guidelines.

Security

Testing

  • Changes are covered with new/existing unit tests?
  • Changes are covered with new/existing integration tests?

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

connorlanigan avatar Aug 01 '23 14:08 connorlanigan

The list of comments is not excessive, I only called repeating patterns once

just-boris avatar Aug 02 '23 13:08 just-boris

The properties are not intended to be logged directly - an analytics library rather should use these to derive the values that it needs. For example, the Flashbar's items property contains JSX, which cannot (usefully) be logged. Instead, an analytics library could e.g. log values like these:

{
  infoMessagesCount: items.filter(item => item.type === "info").length,
  errorMessagesCount: items.filter(item => item.type === "error").length,
  serviceHasPersistentErrorStatus: items.some(item => item.type === "error" && !item.dismissible)
}

I did not want to preprocess data like this inside of our components, since that means a) we have to define all useful derived data, which may not cover future use cases, and b) it introduces a risk of becoming another stable API. The currently exposed properties are intentionally just the public component properties, since that API is fixed anyways (with the exception of the Tabs component, where we expose the actual value of the controllable activeTabId property that might be undefined).

connorlanigan avatar Aug 02 '23 14:08 connorlanigan

I see, I can approve this change, because it is internal, but there are some caveats making it harder to revisit if any issues occur.

  1. Changes propagate to production within weeks, so there will be moment when this property may contain different data depending on their dependency freshness
  2. These values may throw when passed into JSON.stringify, this may cause intermittent errors which will be hard to capture
  3. Traversing some structures (side navigation items for example) may be tricky, because they are recursive. I believe we can simplify work for consumer end, preparing more digestable info (for example, annotating individual items)

just-boris avatar Aug 02 '23 14:08 just-boris

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (232149b) 93.55% compared to head (6b78ec0) 93.55%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1398   +/-   ##
=======================================
  Coverage   93.55%   93.55%           
=======================================
  Files         627      627           
  Lines       16859    16859           
  Branches     5568     5568           
=======================================
  Hits        15772    15772           
  Misses       1014     1014           
  Partials       73       73           
Files Changed Coverage Δ
src/select/index.tsx 100.00% <ø> (ø)
src/alert/index.tsx 100.00% <100.00%> (ø)
src/autosuggest/index.tsx 100.00% <100.00%> (ø)
src/breadcrumb-group/index.tsx 100.00% <100.00%> (ø)
src/button-dropdown/index.tsx 100.00% <100.00%> (ø)
src/cards/index.tsx 93.90% <100.00%> (ø)
src/checkbox/index.tsx 100.00% <100.00%> (ø)
src/date-picker/index.tsx 96.61% <100.00%> (ø)
src/date-range-picker/index.tsx 97.11% <100.00%> (ø)
src/icon/index.tsx 100.00% <100.00%> (ø)
... and 17 more

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Aug 02 '23 15:08 codecov[bot]