close
Skip to content

chore: remove uuid dependency#3130

Merged
lahirumaramba merged 4 commits into
firebase:mainfrom
gameroman:replace-uuid-dependency
May 14, 2026
Merged

chore: remove uuid dependency#3130
lahirumaramba merged 4 commits into
firebase:mainfrom
gameroman:replace-uuid-dependency

Conversation

@gameroman
Copy link
Copy Markdown
Contributor

No description provided.

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

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.

Comment thread src/eventarc/eventarc-utils.ts
@gameroman
Copy link
Copy Markdown
Contributor Author

/gemini review

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

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.

Comment thread src/eventarc/eventarc-utils.ts Outdated
Comment thread src/eventarc/eventarc-utils.ts Outdated
@lahirumaramba lahirumaramba self-assigned this May 13, 2026
@lahirumaramba lahirumaramba force-pushed the replace-uuid-dependency branch from 66adf06 to 030fb1e Compare May 14, 2026 17:54
@lahirumaramba
Copy link
Copy Markdown
Member

@gemini-code-assist review

@lahirumaramba lahirumaramba force-pushed the replace-uuid-dependency branch from 030fb1e to edeb9b8 Compare May 14, 2026 17:59
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

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.

Comment on lines 20 to +21
import * as validator from '../utils/validator';
import { randomUUID } from 'node:crypto';
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

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.

Suggested change
import * as validator from '../utils/validator';
import { randomUUID } from 'node:crypto';
import { randomUUID } from 'node:crypto';
import * as validator from '../utils/validator';

Comment on lines 29 to +30
import { clone } from 'lodash';
import * as crypto from 'crypto';
import * as crypto from 'node:crypto';
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

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.

Suggested change
import { clone } from 'lodash';
import * as crypto from 'crypto';
import * as crypto from 'node:crypto';
import * as crypto from 'node:crypto';
import { clone } from 'lodash';

Copy link
Copy Markdown
Member

@lahirumaramba lahirumaramba left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution! LGTM!

@lahirumaramba lahirumaramba merged commit 2c8d215 into firebase:main May 14, 2026
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants