fix(core): improve local registry setup and make it more flexible #31482
Conversation
FrozenPandaz
requested review from
meeroslav,
vsavkin,
mandarini and
a team
as code owners
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
View your CI Pipeline Execution ↗ for commit c21faf7.
☁️ Nx Cloud last updated this comment at |
e2e/utils/global-setup.ts
Outdated
| // Keep checking until the configured registry changes from default | ||
| while (currentRegistry === defaultRegistry) { | ||
| console.log( | ||
| 'Waiting for registry configuration to change from default...' |
There was a problem hiding this comment.
Maybe include what the default is in the log to avoid any confusion
There was a problem hiding this comment.
@claude can you include the default in the log please?
There was a problem hiding this comment.
Claude encountered an error —— View job
Todo List
- Read the current file to understand the log message that needs updating
- Identify what the default registry value is
- Update the log message to include the default value
- Commit the change to the PR branch
Summary
I've successfully updated the log message on line 41 of e2e/utils/global-setup.ts to include the default registry value for better clarity.
Change Made:
- 'Waiting for registry configuration to change from default...'
+ `Waiting for registry configuration to change from default (${defaultRegistry})...`This will now log: Waiting for registry configuration to change from default (https://registry.npmjs.org/)... making it clear what the "default" registry is.
The change has been committed to the PR branch: 72fa6f0
There was a problem hiding this comment.
@claude Instead of printing default (https://registry.npmjs.org/), just print the default URL.
There was a problem hiding this comment.
Claude finished @FrozenPandaz's task in 54s —— View job
Todo List
- Read the current file to understand the log message that needs updating
- Update the log message to print just the default URL (remove "default" text and parentheses)
- Commit the change to the PR branch
Summary
I've successfully updated the log message in e2e/utils/global-setup.ts on line 41 to simplify the output as requested.
Change Made:
- `Waiting for registry configuration to change from default (${defaultRegistry})...`
+ `Waiting for registry configuration to change from ${defaultRegistry}...`Now the log will print just the URL (e.g., Waiting for registry configuration to change from https://registry.npmjs.org/...) without the extra "default" text and parentheses.
The change has been committed to the PR branch: c184bea
FrozenPandaz
force-pushed
the
fix-e2e-local
branch
from
c184bea to
49395dd
Compare
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| function parseRegistryUrl(registryUrl: string): { | ||
| hostname: string; | ||
| port: string; | ||
| } { | ||
| try { | ||
| const url = new URL(registryUrl); | ||
| return { | ||
| hostname: url.hostname, | ||
| port: url.port || (url.protocol === 'https:' ? '443' : '80'), | ||
| }; | ||
| } catch { | ||
| // Fallback for invalid URLs | ||
| return { | ||
| hostname: 'localhost', | ||
| port: '4873', | ||
| }; | ||
| } |
There was a problem hiding this comment.
The parseRegistryUrl function may not handle protocol-less URLs correctly. The URL constructor requires a protocol to properly parse URLs. Consider adding validation to handle cases like localhost:4873 by prepending http:// when no protocol is detected:
function parseRegistryUrl(registryUrl: string): {
hostname: string;
port: string;
} {
try {
// Add protocol if missing
const urlWithProtocol = registryUrl.includes('://')
? registryUrl
: `http://${registryUrl}`;
const url = new URL(urlWithProtocol);
// Rest of the function...
}
// ...
}This would make the function more robust when handling various registry URL formats.
| function parseRegistryUrl(registryUrl: string): { | |
| hostname: string; | |
| port: string; | |
| } { | |
| try { | |
| const url = new URL(registryUrl); | |
| return { | |
| hostname: url.hostname, | |
| port: url.port || (url.protocol === 'https:' ? '443' : '80'), | |
| }; | |
| } catch { | |
| // Fallback for invalid URLs | |
| return { | |
| hostname: 'localhost', | |
| port: '4873', | |
| }; | |
| } | |
| function parseRegistryUrl(registryUrl: string): { | |
| hostname: string; | |
| port: string; | |
| } { | |
| try { | |
| // Add protocol if missing | |
| const urlWithProtocol = registryUrl.includes('://') | |
| ? registryUrl | |
| : `http://${registryUrl}`; | |
| const url = new URL(urlWithProtocol); | |
| return { | |
| hostname: url.hostname, | |
| port: url.port || (url.protocol === 'https:' ? '443' : '80'), | |
| }; | |
| } catch { | |
| // Fallback for invalid URLs | |
| return { | |
| hostname: 'localhost', | |
| port: '4873', | |
| }; | |
| } | |
| } |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
There was a problem hiding this comment.
It's okay, the registry URL will for sure have the protocol.
Wait for registry configuration to change from default instead of hardcoded local registry. Add proper process management for local registry startup with cleanup. Improve error handling and make registry configuration work with both local and external registries.
Co-authored-by: FrozenPandaz <[email protected]>
Co-authored-by: FrozenPandaz <[email protected]>
FrozenPandaz
force-pushed
the
fix-e2e-local
branch
from
49395dd to
c21faf7
Compare
FrozenPandaz
added
the
priority: medium
Current Behavior
The E2E global setup hardcodes local registry configuration and waits for a specific localhost registry to be available, which can cause issues when the registry setup is managed differently or when using external registries.
Expected Behavior
The E2E setup should be more flexible and work with different registry configurations:
Related Issue(s)
This PR improves the E2E local testing setup to be more robust and flexible.
Changes Made