Uh oh!
There was an error while loading. Please reload this page.
chore: remove uuid dependency - #3130
Conversation
There was a problem hiding this comment.
Code Review
This pull request replaces the external uuid dependency with the native crypto.randomUUID() method. A critical issue was identified in src/eventarc/eventarc-utils.ts where the crypto module is used without an explicit import, which will lead to a ReferenceError in Node.js 18 environments.
Uh oh!
There was an error while loading. Please reload this page.
gameroman
commented
May 7, 2026
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request removes the uuid dependency and replaces its usage with the native crypto.randomUUID() method in src/eventarc/eventarc-utils.ts and unit tests. Review feedback suggests improving the implementation by using the node: prefix for built-in module imports and adopting named imports for better clarity.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
66adf06 to
030fb1eComparelahirumaramba
commented
May 14, 2026
@gemini-code-assist review |
030fb1e to
edeb9b8CompareThere was a problem hiding this comment.
Code Review
This pull request replaces the external uuid dependency with the built-in node:crypto module for generating UUIDs in eventarc-utils.ts and condition-evaluator.spec.ts. Correspondingly, uuid and its types were removed from package.json. Review feedback suggests reordering imports to ensure built-in Node.js modules are grouped before external or local dependencies to improve code organization and readability.
| import * as validator from '../utils/validator'; | ||
| import { randomUUID } from 'node:crypto'; |
There was a problem hiding this comment.
Following standard TypeScript conventions, built-in modules (like node:crypto) should be grouped together and placed before local relative imports. This improves readability and maintainability by clearly separating external/built-in dependencies from internal project files.
| import*asvalidatorfrom'../utils/validator'; | |
| import{randomUUID}from'node:crypto'; | |
| import{randomUUID}from'node:crypto'; | |
| import*asvalidatorfrom'../utils/validator'; |
| import { clone } from 'lodash'; | ||
| import * as crypto from 'crypto'; | ||
| import * as crypto from 'node:crypto'; |
There was a problem hiding this comment.
Built-in Node.js modules should ideally be grouped at the top of the import section, before external dependencies like lodash. This follows common style guides for organizing imports in TypeScript projects.
| import{clone}from'lodash'; | |
| import*ascryptofrom'crypto'; | |
| import*ascryptofrom'node:crypto'; | |
| import*ascryptofrom'node:crypto'; | |
| import{clone}from'lodash'; |
lahirumaramba
left a comment
There was a problem hiding this comment.
Thank you for your contribution! LGTM!
No description provided.