build(websocket): migrate to ESM#41680
Conversation
Signed-off-by: hainenber <dotronghai96@gmail.com>
Signed-off-by: hainenber <dotronghai96@gmail.com>
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
|
|
||
| import { createLogger } from './logger'; | ||
| import { buildConfig, RedisConfig } from './config'; | ||
| import { createLogger } from './logger.js'; |
There was a problem hiding this comment.
Suggestion: The relative import now points to ./logger.js from a .ts source file, but the dev-server script runs sources directly (node --experimental-strip-types src/index.ts), where src/logger.js does not exist. This will fail module resolution at startup in dev mode; use source-resolvable specifiers for direct-TS execution or stop running TS sources directly. [api mismatch]
Severity Level: Critical 🚨
- ❌ Dev server `npm run dev-server` fails to start.
- ⚠️ Local websocket development and debugging blocked for contributors.Steps of Reproduction ✅
1. In `superset-websocket/package.json:7–15`, note the `dev-server` script:
`\"dev-server\": \"node --experimental-strip-types src/index.ts start\"` (line 13), which
executes `src/index.ts` directly under Node with experimental type stripping, without
building JS output.
2. In `superset-websocket/README.md:9–20`, the documented way to run locally is `npm run
dev-server` (lines 11–15), so contributors will routinely start the service via this
direct-TS path.
3. In `superset-websocket/src/index.ts:19–31`, the file executed by the dev-server script,
observe the ESM-style relative import `import { createLogger } from './logger.js';` on
line 29, alongside `import { buildConfig, RedisConfig } from './config.js';` on line 30.
4. In `superset-websocket/src/logger.ts` (lines 19–52) and the `src` directory listing
(`ls superset-websocket/src` showing only `config.ts`, `index.ts`, `logger.ts`), confirm
there is no `logger.js` (nor any JS files) in `src/`. When running `npm run dev-server`,
Node resolves `./logger.js` relative to `src/index.ts`, fails to find a matching file, and
throws a module resolution error before the WebSocket server starts, breaking local dev
startup.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-websocket/src/index.ts
**Line:** 29:29
**Comment:**
*Api Mismatch: The relative import now points to `./logger.js` from a `.ts` source file, but the `dev-server` script runs sources directly (`node --experimental-strip-types src/index.ts`), where `src/logger.js` does not exist. This will fail module resolution at startup in dev mode; use source-resolvable specifiers for direct-TS execution or stop running TS sources directly.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
|
The flagged issue is correct. When using Node.js with To resolve this while maintaining the ESM-compliant import style required by the new configuration, you should ensure the development environment or the build process correctly maps these imports, or adjust the import to be compatible with the runtime. Given the project is moving to ESM, the standard approach is to use the Since you are using I have checked the PR comments, and there are no other pending review comments to address. Would you like me to help you explore alternative ways to configure the development server to support this ESM-style resolution? superset-websocket/src/index.ts |
Code Review Agent Run #8ccb1aActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
|
The ESM migration itself reads right, but the It's also conflicting with master, so it'll want a rebase before we merge. |
Signed-off-by: hainenber <dotronghai96@gmail.com>
Signed-off-by: hainenber <dotronghai96@gmail.com>
|
Thanks Evan, I missed that detail on |
| */ | ||
|
|
||
| import { merge as _merge } from 'lodash'; | ||
| import { merge as _merge } from 'lodash-es'; |
There was a problem hiding this comment.
Suggestion: This file is now explicitly ESM, but configFromFile() still uses CommonJS require(), which will throw ReferenceError: require is not defined at runtime when loading config. Replace the config file loading path with an ESM-compatible approach (for example createRequire(import.meta.url) or fs + JSON.parse) so startup does not fail. [api mismatch]
Severity Level: Critical 🚨
❌ Websocket service ignores config.json/config.test.json values.
⚠️ Vitest config tests fail, blocking CI success.
⚠️ JWT secret from file never applied to opts.
⚠️ Redis/statds tuning from file never applied.Steps of Reproduction ✅
1. Open `/workspace/superset/superset-websocket/package.json` where line 5 sets `"type":
"module"` and `/workspace/superset/superset-websocket/tsconfig.json` where line 6 sets
`"module": "nodenext"`, confirming the compiled JS for this service runs as native ESM.
2. Start the websocket service via `npm run start` (script defined at `package.json:7` as
`node dist/index.js start`), which loads the compiled `dist/index.js` entrypoint from
`/workspace/superset/superset-websocket/src/index.ts`.
3. In `/workspace/superset/superset-websocket/src/index.ts` lines 18–23, `environment` and
`startServer` are computed, then `export const opts = buildConfig();` is executed,
importing `buildConfig` from `/workspace/superset/superset-websocket/src/config.ts:192`.
4. In `/workspace/superset/superset-websocket/src/config.ts` lines 192–195,
`buildConfig()` calls `defaultConfig()` and `configFromFile()`, then passes the result
through `applyEnvOverrides()`, so `configFromFile()` runs on every startup and for every
test that calls `buildConfig()`.
5. In the same file, `configFromFile()` at lines 103–107 computes `configFile` as
`'../config.json'` or `'../config.test.json'`, then executes `return require(configFile);`
inside a `try` block; under ESM (package `"type": "module"`), `require` is not defined, so
this line throws `ReferenceError: require is not defined`, which is caught by the `catch`
at lines 108–111, causing a `console.warn('config.json file not found')` and returning
`{}` instead of the JSON contents even when
`/workspace/superset/superset-websocket/config.test.json` exists.
6. When running tests via `npm test` (script at `package.json:9`), `spec/config.test.ts`
line 22 calls `buildConfig()` and expects values sourced from `config.test.json` (e.g.,
`jwtSecret`, redis `db`, statsd host/port). Because `configFromFile()` now always returns
`{}` due to the ESM `require` failure, these expectations no longer match, causing the
config tests to fail and CI to report red; similarly, in production the service will
silently ignore `config.json` and run with defaults plus env vars only.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-websocket/src/config.ts
**Line:** 20:20
**Comment:**
*Api Mismatch: This file is now explicitly ESM, but `configFromFile()` still uses CommonJS `require()`, which will throw `ReferenceError: require is not defined` at runtime when loading config. Replace the config file loading path with an ESM-compatible approach (for example `createRequire(import.meta.url)` or `fs` + `JSON.parse`) so startup does not fail.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fixThere was a problem hiding this comment.
can JSON.parse(readFileSync("/path/to/file")) for this case too?
There was a problem hiding this comment.
oh, nvm. Saw your 2nd suggestion now.
There was a problem hiding this comment.
Yes — JSON.parse(readFileSync("/path/to/file")) can work if readFileSync returns a string, or you convert the buffer first.
In Node, readFileSync() without an encoding returns a Buffer, so the safe version is:
import { readFileSync } from 'fs';
const config = JSON.parse(readFileSync(configFile, 'utf8'));That’s a good ESM-friendly replacement here and avoids require() entirely.
Why this is appropriate
- Works in native ESM
- Keeps the config loading simple
- Still supports
config.json/config.test.jsonat startup and in tests
So yes, this is a valid fix for the issue raised in the thread.
There was a problem hiding this comment.
Fixed in latest commit
…or cross-module config load Signed-off-by: hainenber <dotronghai96@gmail.com>
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
| const configFile = isTest ? '../config.test.json' : '../config.json'; | ||
| try { | ||
| return require(configFile); | ||
| return JSON.parse(readFileSync(configFile, 'utf8')) as ConfigType; |
There was a problem hiding this comment.
Suggestion: Using readFileSync with configFile as a bare relative path resolves against process.cwd(), not against config.ts/dist/config.js. This changes behavior from module-relative loading and will miss superset-websocket/config.test.json (and production config.json) when the process is started from the package root, causing file-based config to be silently skipped. Resolve the path from import.meta.url (or equivalent module directory logic) before reading. [api mismatch]
Severity Level: Critical 🚨
- ❌ WebSocket sidecar container fails startup despite config.json present.
- ❌ JWT secret from config.json never applied to server options.
- ⚠️ Test config.test.json ignored; tests use default connection settings.Steps of Reproduction ✅
1. Start the `superset-websocket` service using Docker Compose, which defines the
`superset-websocket` container at `docker-compose.yml:18-47` and mounts a config file to
`/home/superset-websocket/config.json` via the volume at `docker-compose.yml:41-43`. The
Dockerfile at `superset-websocket/Dockerfile:40-51` sets `WORKDIR
/home/superset-websocket` and runs `CMD ["npm", "start"]`, which uses the `start` script
in `superset-websocket/package.json:7-8` (`"start": "node dist/index.js start"`), so
Node’s `process.cwd()` is `/home/superset-websocket`.
2. When `dist/index.js` runs, it imports `./config.js` and immediately calls
`buildConfig()` via `export const opts = buildConfig();` at
`superset-websocket/src/index.ts:29-31,81`. `buildConfig()` in
`superset-websocket/src/config.ts:193-195` merges `defaultConfig()` with
`configFromFile()`.
3. Inside `configFromFile()` at `superset-websocket/src/config.ts:104-109`, `NODE_ENV` is
`production`, so `isTest` is false and `configFile` is set to `'../config.json'` at line
106. The call `readFileSync(configFile, 'utf8')` at line 108 resolves `'../config.json'`
relative to `process.cwd()` (`/home/superset-websocket`), so Node attempts to read
`/home/config.json`, but the actual file is at `/home/superset-websocket/config.json`.
This causes `readFileSync` to throw `ENOENT`, the `catch` block at lines 109-111 logs
`config.json file not found`, and `configFromFile()` returns `{}`, silently discarding the
mounted configuration.
4. Because file configuration is ignored, `buildConfig()` returns only `defaultConfig()`
and `opts.jwtSecret` remains the empty string from `defaultConfig()`
(`superset-websocket/src/config.ts:70-72`). With `startServer` true (`process.argv[2] ===
'start'` at `superset-websocket/src/index.ts:79`), the startup check at
`superset-websocket/src/index.ts:98-102` detects `opts.jwtSecret.length < 32`, logs
`Please provide a JWT secret at least 32 bytes long`, and calls `process.exit(1)`, causing
the `superset-websocket` container to fail to start even though a valid `config.json` (or
`config.example.json`) is present and mounted.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-websocket/src/config.ts
**Line:** 108:108
**Comment:**
*Api Mismatch: Using `readFileSync` with `configFile` as a bare relative path resolves against `process.cwd()`, not against `config.ts`/`dist/config.js`. This changes behavior from module-relative loading and will miss `superset-websocket/config.test.json` (and production `config.json`) when the process is started from the package root, causing file-based config to be silently skipped. Resolve the path from `import.meta.url` (or equivalent module directory logic) before reading.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fixThere was a problem hiding this comment.
Fixed in latest commit
Signed-off-by: hainenber <dotronghai96@gmail.com>
Signed-off-by: hainenber <dotronghai96@gmail.com>
Code Review Agent Run #cfe637Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
rusackas
left a comment
rusackas
left a comment
There was a problem hiding this comment.
Nice work, @hainenber, LGTM. Went through the codeant flags and the fixes all check out (build-before-start, the require() swap, import.meta.dirname for the path). CI's green, approving.
2 participants
build(websocket): migrate to ESM
SUMMARY
I was trying to resolve failed CI in a Dependabot PR that bumps
cookieto v2 and come to realization that the library now ESM-only. Hence this PR to migrate wholesuperset-websocketservice to ESM.It's a backend service so expect no regression/backward-incompat after migration.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A
TESTING INSTRUCTIONS
Green CI for acceptance criteria
ADDITIONAL INFORMATION