flowing env vars to the client app for inspector #258
Conversation
This allows you to set the SERVER_PORT or MCP_PROXY_FULL_ADDRESS as an environment variable and the value will be injected into the static app when it's run. Fixes modelcontextprotocol#256
| import http from "http"; | ||
| import { dirname, join } from "path"; | ||
| import handler from "serve-handler"; | ||
| import { fileURLToPath } from "url"; | ||
|
|
||
| const __dirname = dirname(fileURLToPath(import.meta.url)); | ||
| const distPath = join(__dirname, "../dist"); | ||
|
|
||
| const server = http.createServer((request, response) => { |
There was a problem hiding this comment.
Would a better approach be to expose an endpoint in this server to return the config ? That would simplify this part of the code (and not have to deal with runtime injection).
Something like that is being done here (although it contacts the proxy server):
Line 228 in ce1a9d3
There was a problem hiding this comment.
The runtime config stuff further down in the client code definitely would be simpler.
There was a problem hiding this comment.
As in, create an API in the client/bin/cli.js that provides an endpoint in which you load the config file? It would but doesn't that add overhead to the startup of the app in that you have to make an API call to get the config, then make an API call to get the config from the server.
Maybe they could be combined, make the /config exist on the client app, which then proxies the SERVER/config.
There was a problem hiding this comment.
@aaronpowell Could we get a mermaid sequence diagram showing how this is intended to work? That would give us a better starting point for understanding how best to plumb this.
There was a problem hiding this comment.
sequenceDiagram
participant SPA
participant ClientHost
participant ProxyServer
Note over SPA: Application starts
SPA->>ClientHost: GET /config
activate ClientHost
ClientHost->>ProxyServer: GET /config
activate ProxyServer
ProxyServer-->>ClientHost: Return configuration data
deactivate ProxyServer
ClientHost-->>SPA: Return configuration data
deactivate ClientHost
Note over SPA: Process configuration
With this we can simplify the "Client Host" (the server run from client/bin/cli.js to not have to worry about much heavy lifting other than "where is the server endpoint" and pass through the request.
There was a problem hiding this comment.
Isn't the proxy server address part of the config?
client/src/App.tsx
Outdated
| : DEFAULT_INSPECTOR_CONFIG; | ||
|
|
||
| // Override with runtime injected values if available | ||
| if (runtimeConfig.MCP_PROXY_FULL_ADDRESS) { |
There was a problem hiding this comment.
Shouldn't this be more like:
if (runtimeConfig.MCP_PROXY_FULL_ADDRESS) {
configFromStorage = {
...configFromStorage,
MCP_PROXY_FULL_ADDRESS: {
value: runtimeConfig.MCP_PROXY_FULL_ADDRESS,
},
};There was a problem hiding this comment.
are there properties other than value on MCP_PROXY_FULL_ADDRESS that are important?
| import http from "http"; | ||
| import { dirname, join } from "path"; | ||
| import handler from "serve-handler"; | ||
| import { fileURLToPath } from "url"; | ||
|
|
||
| const __dirname = dirname(fileURLToPath(import.meta.url)); | ||
| const distPath = join(__dirname, "../dist"); | ||
|
|
||
| const server = http.createServer((request, response) => { |
There was a problem hiding this comment.
The runtime config stuff further down in the client code definitely would be simpler.
cliffhall
added
enhancement
| import http from "http"; | ||
| import { dirname, join } from "path"; | ||
| import handler from "serve-handler"; | ||
| import { fileURLToPath } from "url"; | ||
|
|
||
| const __dirname = dirname(fileURLToPath(import.meta.url)); | ||
| const distPath = join(__dirname, "../dist"); | ||
|
|
||
| const server = http.createServer((request, response) => { |
There was a problem hiding this comment.
As in, create an API in the client/bin/cli.js that provides an endpoint in which you load the config file? It would but doesn't that add overhead to the startup of the app in that you have to make an API call to get the config, then make an API call to get the config from the server.
Maybe they could be combined, make the /config exist on the client app, which then proxies the SERVER/config.
client/src/App.tsx
Outdated
| : DEFAULT_INSPECTOR_CONFIG; | ||
|
|
||
| // Override with runtime injected values if available | ||
| if (runtimeConfig.MCP_PROXY_FULL_ADDRESS) { |
There was a problem hiding this comment.
are there properties other than value on MCP_PROXY_FULL_ADDRESS that are important?
client/bin/cli.js
Outdated
|
|
||
| // Create a runtime config object with environment variables | ||
| const runtimeConfig = { | ||
| MCP_PROXY_FULL_ADDRESS: process.env.MCP_PROXY_FULL_ADDRESS || "", |
There was a problem hiding this comment.
MCP_PROXY_FULL_ADDRESS and MCP_PROXY_PORT should be ConfigItems.
This means that when you set environment variables we read them on the server and add them to the config info obtained by the proxy, resulting in the proxy server being the 'winner' in terms of what the config should look like.
aaronpowell
force-pushed
the
aaronpowell/issue-256
branch
from
e041581 to
d76a79a
Compare
This allows you to set the SERVER_PORT or MCP_PROXY_FULL_ADDRESS as an environment variable and the value will be injected into the static app when it's run. Also making the transport type provided by the config.
Fixes #256 and #255
Motivation and Context
How Has This Been Tested?
Breaking Changes
Types of changes
Checklist
Additional context