feat(common): Remove uuid and update unique id generation #1408

NicholasBoll · 2022-01-05T23:41:56Z

Summary

Our current id generation has issues.

It generates very long ids that add noise to the page
It is possible to generate a blank id
Id generation is slow
Snapshot tests require mocking the uuid module

Release Note

NOTE for jest snapshots: This change removes the uuid package and instead will generate a one-time client seed and then create auto-incrementing ids. This change will not break UI or automated UI tests. It will break snapshot tests however. Previously, the only way to get stable ids for snapshot tests was to mock the uuid module. This was an implementation detail. To make snapshots work again, add the following to your jest setup file:

import {setUniqueSeed, resetUniqueIdCount} from '@workday/canvas-kit-react/common';

beforeEach(() => {
  setUniqueSeed('a'); // force set the seed
  resetUniqueIdCount(); // reset the unique id count
});

This will ensure each Jest snapshot has ids that look like a0 and a1 and will be the same every time the snapshot is run. Do not use these methods in production though - it may lead to inaccessible applications due to IDREF collisions.

Additional References

Test of different ID generation methods https://codesandbox.io/s/tkod6

Remaining dependencies on older versions of uuid:

request - this is a deprecated lib that we use for scripts
node-sass - We're using v4. Newest is v7. This PR removed request which is using uuid@3, but hasn't been released yet: Use make-fetch-happen instead of request sass/node-sass#3193. Currently blocked. sass-loader that is compatible with node-sass@7 requires webpack@5 which Storybook doesn't currently support.
jest-unit - We're using v10. Current version is v13
lerna - We're using v3. Current version is v4
react-docgen-typescript-loader - We're on v3.3.0. Latest is v3.7.2
storybook/addon-docs - Oh, Storybook...

Fixes: Workday#1311, Workday#1106, Workday#664, Workday#602

NicholasBoll · 2022-01-05T23:49:44Z

cypress/integration/Combobox.spec.ts

@@ -37,7 +37,7 @@ describe('Combobox', () => {
    });

    it('should not show menu', () => {
-      cy.findByRole('listbox').should('not.be.visible');
+      cy.findByRole('listbox').should('not.exist');


All these were due to a breaking change in Cypress v6.

This "fix" did find legitimate issues with our tests, but I feel like we've lost some semantics. I want to know if the element is no longer accessible to the user. Whether it is still in the DOM or just hidden is an implementation detail. Perhaps I'll make an be.inaccessible assertion that does this.

NicholasBoll · 2022-01-05T23:52:20Z

cypress/integration/Tooltip.spec.ts

@@ -155,14 +155,14 @@ describe('Tooltip', () => {
    });

    it('the span element should not have an aria-describedby attribute', () => {
-      cy.get('button').should('not.have.attr', 'aria-describedby');
+      cy.get('span').should('not.have.attr', 'aria-describedby');


This was caught by the change in Cypress v6. Before, not.have.attr would pass even if cy.get('button') never actually matched anything. This was a typo from long ago that Cypress was perfectly happy to incorrectly pass.

NicholasBoll · 2022-01-05T23:52:55Z

cypress/integration/Tooltip.spec.ts

    });

-    context('when the "Delete" button is hovered', () => {
+    context('when the "Some Text" text is hovered', () => {


This was a copy/paste error. The tooltip isn't from a delete button, but a span with some text.

NicholasBoll · 2022-01-05T23:55:51Z

cypress/support/commands.ts

-  if (typeof url === 'object') {
-    // eslint-disable-next-line no-param-reassign
-    url = options.url;
+Cypress.Commands.overwrite('visit', (originalFn, urlOrOptions, inputOptions = {}) => {


Changes in this file were from Cypress adding type defintions to Cypress.Commands.add and Cypress.Commands.overwrite in Cypress v9. In most cases, this is an improvement. In the case of commands that use Typescript overrides, this causes problems. I've rearranged command overrides and added some any in cases where the Cypress types introduce problems that cannot be resolved.

cypress · 2022-01-05T23:59:09Z

Test summary

666 • 0 • 1 • 0 • 0

Run details


Project	canvas-kit
Status	Passed
Commit	`d8b12e9` ℹ️
Started	Jan 7, 2022 10:53 PM
Ended	Jan 7, 2022 10:59 PM
Duration	05:26 💡
OS	Linux Ubuntu - 20.04
Browser	Electron 94

View run in Cypress Dashboard ➡️

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

NicholasBoll · 2022-01-06T00:04:38Z

cypress/support/cypress-plugin-tab.d.ts

@@ -1,5 +1,5 @@
 declare namespace Cypress {
-  interface Chainable {
-    tab(options?: Partial<{shift: Boolean}>): Chainable;
+  interface Chainable<Subject = any> {


This was the correct typing all along

NicholasBoll · 2022-01-06T00:06:10Z

cypress/support/index.ts

+const resizeObserverLoopErrRe = /^[^(ResizeObserver loop limit exceeded)]/;
+Cypress.on('uncaught:exception', err => {
+  /* returning false here prevents Cypress from failing the test */
+  if (resizeObserverLoopErrRe.test(err.message)) {
+    return false;
+  }
+});


The ResizeObserver loop limit exceeded error started happening after upgrading Cypress from 5.6.0 to 9.2.0. I'm not sure which Cypress version introduced the error. Perhaps later we can find out.

NicholasBoll · 2022-01-06T00:07:36Z

jest/setupTests.ts

+beforeEach(() => {
+  setUniqueSeed('a');
+  resetUniqueIdCount();
+});


This isn't actually needed for our tests, but it is nice to have something to point to as an example for teams that need this setup.

I'm sure teams are gonna wanna use this, nice addition

Should something like this go into the testing docs for reference?

nvm you documented this pretty well in the readme

NicholasBoll · 2022-01-06T00:09:45Z

modules/preview-react/index.ts

@@ -6,3 +6,5 @@ export * from './select';
 export * from './side-panel';
 export * from './text-area';
 export * from './text-input';
+
+/// <reference types="@types/node" />


This removes Typescript errors complaining that process doesn't exist. uuid added @types/node to the global namespace for us.

mannycarrera4 · 2022-01-06T18:15:03Z

modules/react/common/lib/utils/useUniqueId.ts

+let seed = Math.random()
+  .toString(36)
+  .slice(2)
+  .replace(/[0-9]*/, '')


did you want to add the - here? I remember the code sandbox you added it.

I added it to the uuid generation. Base 36 doesn't include dashes.

mannycarrera4

Looks good to me just some minor comments/ questions

…uid issues

NicholasBoll added 2 commits January 4, 2022 12:10

chore: Remove uuid and update unique id generation

c120a03

Fixes: Workday#1311, Workday#1106, Workday#664, Workday#602

Add tests, documentation, and fix type errors

b983bfc

This was linked to issues Jan 5, 2022

Bump uuid dependency #1311

Closed

Add a ID utility function for generating valid IDREFs #602

Closed

Generate stable IDs #664

Closed

UUID dep is out of date and throwing a warning on install #1106

Closed

NicholasBoll commented Jan 5, 2022

View reviewed changes

NicholasBoll added 2 commits January 5, 2022 16:50

Remove a .only

81e7828

Remove unwanted change

9041420

NicholasBoll commented Jan 5, 2022

View reviewed changes

NicholasBoll commented Jan 6, 2022

View reviewed changes

Fix mistakes

e976ed0

mannycarrera4 reviewed Jan 6, 2022

View reviewed changes

mannycarrera4 approved these changes Jan 6, 2022

View reviewed changes

NicholasBoll added 4 commits January 7, 2022 09:16

Add blurb in our testing doc about stabile snapshot tests

dbc9535

Remove request and update jest, jest-junit, and node-sass to remove u…

b581fdc

…uid issues

Fix resolution of node-sass

6e38744

Upgrade rollup to remove multiple @types/node

01d45d9

NicholasBoll force-pushed the chore/remove-uuid branch from 2d5c111 to 01d45d9 Compare January 7, 2022 21:13

NicholasBoll added 3 commits January 7, 2022 14:24

Remove extra @types/node

8be6b18

Remove redundant types

3655f6a

Removed the babel message

3923ae4

NicholasBoll added the automerge label Jan 7, 2022

workday-canvas-kit enabled auto-merge (squash) January 7, 2022 22:50

workday-canvas-kit merged commit 9efce13 into Workday:prerelease/minor Jan 7, 2022

This was referenced Jan 10, 2022

Bump uuid dependency #1311

Closed

UUID dep is out of date and throwing a warning on install #1106

Closed

Generate stable IDs #664

Closed

Add a ID utility function for generating valid IDREFs #602

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(common): Remove uuid and update unique id generation #1408

feat(common): Remove uuid and update unique id generation #1408

NicholasBoll commented Jan 5, 2022 •

edited

NicholasBoll Jan 5, 2022

NicholasBoll Jan 5, 2022

NicholasBoll Jan 5, 2022

NicholasBoll Jan 5, 2022

cypress bot commented Jan 5, 2022 •

edited

NicholasBoll Jan 6, 2022

NicholasBoll Jan 6, 2022

NicholasBoll Jan 6, 2022

mannycarrera4 Jan 6, 2022

mannycarrera4 Jan 6, 2022

mannycarrera4 Jan 6, 2022

NicholasBoll Jan 6, 2022

mannycarrera4 Jan 6, 2022

NicholasBoll Jan 6, 2022

mannycarrera4 left a comment

feat(common): Remove uuid and update unique id generation #1408

feat(common): Remove uuid and update unique id generation #1408

Conversation

NicholasBoll commented Jan 5, 2022 • edited

Summary

Release Note

Additional References

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cypress bot commented Jan 5, 2022 • edited

Test summary

Run details

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mannycarrera4 left a comment

Choose a reason for hiding this comment

NicholasBoll commented Jan 5, 2022 •

edited

cypress bot commented Jan 5, 2022 •

edited