-
Notifications
You must be signed in to change notification settings - Fork 98
fix(world-local): add baseUrl support for HTTPS and custom hostnames #245
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(world-local): add baseUrl support for HTTPS and custom hostnames #245
Conversation
The world-local package was hardcoded to use http://localhost: when making queue requests, which failed when: - Dev server runs on HTTPS (e.g., next dev --experimental-https) - Dev server uses custom hostname (e.g., local.fanbeam.app) Changes: - Add baseUrl to Config type with WORKFLOW_EMBEDDED_BASE_URL env var support - Refactor createEmbeddedWorld to accept Partial<Config> instead of individual params - Refactor createQueue to accept full Config instead of just port - Update queue implementation to use config.baseUrl instead of hardcoded URL This allows full control over the queue endpoint URL while maintaining backward compatibility with existing code.
|
|
@aryasaatvik is attempting to deploy a commit to the Vercel Labs Team on Vercel. A member of the Team first needs to authorize it. |
| * @param {Partial<Config>} args - The configuration to use for the embedded world. | ||
| */ | ||
| export function createEmbeddedWorld({ | ||
| dataDir, | ||
| port, | ||
| }: { | ||
| dataDir?: string; | ||
| port?: number; | ||
| }): World { | ||
| const dir = dataDir ?? config.value.dataDir; | ||
| const queuePort = port ?? config.value.port; | ||
| export function createEmbeddedWorld(args: Partial<Config>): World { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| * @param {Partial<Config>} args - The configuration to use for the embedded world. | |
| */ | |
| export function createEmbeddedWorld({ | |
| dataDir, | |
| port, | |
| }: { | |
| dataDir?: string; | |
| port?: number; | |
| }): World { | |
| const dir = dataDir ?? config.value.dataDir; | |
| const queuePort = port ?? config.value.port; | |
| export function createEmbeddedWorld(args: Partial<Config>): World { | |
| * @param {Partial<Config>} [args] - Optional configuration to use for the embedded world. | |
| */ | |
| export function createEmbeddedWorld(args?: Partial<Config>): World { |
The createEmbeddedWorld function parameter was required but should be optional, as the function uses default configuration from environment variables and can be called without arguments per the documentation.
View Details
Analysis
TypeScript compilation error: createEmbeddedWorld() requires argument but documentation shows optional usage
What fails: The createEmbeddedWorld() function in packages/world-local/src/index.ts has a required parameter args: Partial<Config>, but the API documentation at docs/content/docs/deploying/world/embedded-world.mdx (line 214) shows calling it without arguments: const world = createEmbeddedWorld();
How to reproduce:
import { createEmbeddedWorld } from '@workflow/world-local';
// Call without arguments as shown in documentation
const world = createEmbeddedWorld();Result: TypeScript compilation error: error TS2554: Expected 1 arguments, but got 0.
Expected: Function should accept being called without arguments with all configuration coming from environment variables and defaults, as the documentation and internal code behavior suggest (config.value provides defaults internally).
Fix applied: Made args parameter optional with args?: Partial<Config> and updated the JSDoc to indicate the parameter is optional with [args] notation. This aligns the type signature with the intended API documented in the user-facing documentation.
| export function createEmbeddedWorld({ | ||
| dataDir, | ||
| port, | ||
| }: { | ||
| dataDir?: string; | ||
| port?: number; | ||
| }): World { | ||
| const dir = dataDir ?? config.value.dataDir; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When createEmbeddedWorld is called with a port override but without an explicit baseUrl, the port is merged into the config but baseUrl is not recomputed, causing queue requests to silently use the original baseUrl instead of the overridden port.
View Details
📝 Patch Details
diff --git a/packages/world-local/src/index.ts b/packages/world-local/src/index.ts
index 8ab912e..9b0f901 100644
--- a/packages/world-local/src/index.ts
+++ b/packages/world-local/src/index.ts
@@ -12,6 +12,12 @@ import { createStreamer } from './streamer.js';
*/
export function createEmbeddedWorld(args: Partial<Config>): World {
const mergedConfig = { ...config.value, ...(args ?? {}) };
+
+ // If port was explicitly provided but baseUrl wasn't, recompute baseUrl
+ if (args?.port !== undefined && args?.baseUrl === undefined) {
+ mergedConfig.baseUrl = `http://localhost:${args.port}`;
+ }
+
return {
...createQueue(mergedConfig),
...createStorage(mergedConfig.dataDir),
Analysis
Port Parameter Silently Ignored in createEmbeddedWorld
What fails: When calling createEmbeddedWorld({ port: 8080 }) without an explicit baseUrl, the port parameter is silently ignored and queue requests continue to use the baseUrl computed at module load time (e.g., http://localhost:3000).
How to reproduce:
// Module loads with PORT=3000 environment variable
// config.value is memoized at module load time: { port: 3000, baseUrl: 'http://localhost:3000', ... }
// Later, code attempts to override port:
const world = createEmbeddedWorld({ port: 8080 });
// Queue requests are sent to http://localhost:3000 instead of http://localhost:8080What happens vs expected:
- Queue requests in
queue.tsuseconfig.baseUrlwhich remainshttp://localhost:3000even thoughport: 8080was passed tocreateEmbeddedWorld() - Expected behavior: When
portis provided without an explicitbaseUrl, thebaseUrlshould be recomputed to use the new port value
Root cause: The createEmbeddedWorld() function merges arguments with the memoized config.value using object spread ({ ...config.value, ...(args ?? {}) }), which overwrites the port field but leaves baseUrl unchanged. Since queue requests only use config.baseUrl for the fetch URL, the port change has no effect.
|
Thanks @aryasaatvik We're actually trying to fix the port issue in this PR #233 (cc @adriandlam). getting PORT from env var or the config should be a fallback This one confuses me - port shouldn't be in both, the port and the baseUrl? |
|
Also please make sure to fulfill the DCO (https://github.com/vercel/workflow/pull/245/checks?check_run_id=54747630629) for that check to pass :) |
|
Thanks for the feedback @pranaygp! I've addressed your concerns in #260, which combines this PR with #233. The new implementation follows your suggested priority:
Also cleaned up the confusing example - |
Problem
The
world-localpackage was hardcoded to usehttp://localhost:${port}when making queue requests, which fails when:next dev --experimental-https)local.example.com)This caused
TypeError: fetch failedsocket errors because the queue was calling the wrong URL.Example failing setup:
{ "dev": "next dev --experimental-https --hostname local.example.com --port 3000" }Solution
baseUrlconfiguration withWORKFLOW_EMBEDDED_BASE_URLenv var supportcreateEmbeddedWorldto acceptPartial<Config>instead of individual parameterscreateQueueto accept fullConfiginstead of justportconfig.baseUrlinstead of hardcoded URLUsage
Environment variable:
export WORKFLOW_EMBEDDED_BASE_URL=https://local.example.com:3000Programmatically:
Changes
baseUrltoConfigtypegetBaseUrlFromEnv()functionPartial<Config>for better type safety