Janitorial: Remove UUID in favor of crypto#101908
Conversation
Jetpack Cloud live (direct link)
Automattic for Agencies live (direct link)
|
|
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: App Entrypoints (~52 bytes added 📈 [gzipped]) DetailsCommon code that is always downloaded and parsed every time the app is loaded, no matter which route is used. Sections (~16312 bytes added 📈 [gzipped]) DetailsSections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to. Async-loaded Components (~3404 bytes added 📈 [gzipped]) DetailsReact components that are loaded lazily, when a certain part of UI is displayed for the first time. Legend What is parsed and gzip size?Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory. Generated by performance advisor bot at iscalypsofastyet.com. |
|
This PR modifies the release build for the following Calypso Apps: For info about this notification, see here: PCYsg-OT6-p2
To test WordPress.com changes, run |
matticbot
added
[Status] Needs Review
There was a problem hiding this comment.
Changes look good to me, and the tests are simpler now. Thanks for these changes.
I confirm I don't see any import from 'uuid'; in the code.
I see we import the crypto dependency in both server files:
client/server/lib/analytics/index.jsclient/server/middleware/logger.js
I smoke tested the setup/onboarding, My home, DateRange and Range, and attempted to leave a review in plugins. Everything worked as expected. I didn't notice any errors in any page or in the console.
tyxla
left a comment
tyxla
left a comment
There was a problem hiding this comment.
LGTM, great cleanup 👍
Just a few minor thoughts, but nothing blocking IMO.
| class DataMailboxFormField extends MailboxFormFieldBase< string > { | ||
| isVisible = false; | ||
| value = uuid_v4(); | ||
| value = crypto.randomUUID() as string; |
There was a problem hiding this comment.
Bummer that we need to cast to string. crypto.randomUUID() will always return a string after all. Are you sure this is really necessary?
There was a problem hiding this comment.
It doesn't return a normal string, it returns a specific string like ${string}-${string}-${string}-${string}-${string}, and below in the file something tries to do value = normalString and TS complains because normalString doesn't match ${string}-${string}-${string}-${string}-${string}. That's why I cast without remorse 😄 .
| Object.assign( {}, DUMMY_MEDIA[ 0 ], { ID: 'media-1', transient: true } ), | ||
| Object.assign( {}, DUMMY_MEDIA[ 1 ], { ID: 'media-2', transient: true } ), | ||
| Object.assign( {}, DUMMY_MEDIA[ 0 ], { ID: 'media-fake-uuid', transient: true } ), | ||
| Object.assign( {}, DUMMY_MEDIA[ 1 ], { ID: 'media-fake-uuid', transient: true } ), |
There was a problem hiding this comment.
To keep the original intent, should keep -1 and -2 suffixes?
There was a problem hiding this comment.
We could, but I'd have to complicate mocking a global (vs a module) and pollute other tests. I could cache the global, overwrite the mock, then reset the global. But is it worth it?
There was a problem hiding this comment.
It could be worth it in some cases, to differentiate between the various IDs. Theoretically, we could at some point introduce bugs that duplicate IDs and we wouldn't catch them.
| // by using uniqueId, which increments its value within the same session. | ||
| const transientItems = [ | ||
| Object.assign( {}, DUMMY_VIDEO_MEDIA[ 0 ], { ID: 'media-3', transient: true } ), | ||
| Object.assign( {}, DUMMY_VIDEO_MEDIA[ 0 ], { ID: 'media-fake-uuid', transient: true } ), |
test/client/setup-test-framework.js
Outdated
| } ) ); | ||
|
|
||
| // Mock crypto.randomUUID with its Node.js implementation | ||
| global.crypto.randomUUID = () => 'fake-uuid'; |
There was a problem hiding this comment.
That's not great because it'll always be the same and that might cause some weird behaviors. We might want to keep uniqueness in some tests and add further inline mocks that set unique UUIDs.
There was a problem hiding this comment.
That makes sense. I'll do as I originally meant (shown in the comment) and see what tests break and hardcode only those tests.
| "@automattic/calypso-typescript-config": "workspace:^" | ||
| debug: "npm:^4.4.0" | ||
| postcss: "npm:^8.5.3" | ||
| uuid: "npm:^9.0.1" |
There was a problem hiding this comment.
Ah well, it would be nice to do this replacement in Gutenberg at some point, too.
alshakero
added
Groundskeeping
github-actions
bot
removed
the
[Status] Needs Review
4 participants
Proposed Changes
This removes the external
uuidlibrary in favor of nativecrypto.Why are these changes being made?
Getting rid of an external library and get whatever benefits that come with that.
Testing Instructions
I can't possibly write testing steps for everything that changed here. But
serveralso import Node.js'scrypto.