Fix for issue #4807: Include JSON file relative imports in dashboard dev mode compilation#4808
Fix for issue #4807: Include JSON file relative imports in dashboard dev mode compilation#4808StefanOrzu wants to merge 2 commits into
Conversation
… vite-config.ts includes a custom plugin that imports a JSON file from a relative path.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
I have read the CLA Document and I hereby sign the CLA You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot. |
|
Worried about impact? Review this PR in Change Stack to explore blast radius before you approve or request changes. 📝 WalkthroughWalkthroughThis PR modifies the TypeScript compilation utility in the dashboard's Vite configuration to support JSON files. The change expands the source file filter to recognize 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/dashboard/vite/utils/compiler.ts (1)
261-267: ⚡ Quick winSkip AST parsing for JSON files to improve performance and avoid edge cases.
After expanding the filter to include
.jsonfiles (line 261), the function proceeds to parse JSON content as a TypeScript AST (line 267) and search for import/export declarations. Since JSON files cannot contain import or export statements, this parsing step is unnecessary overhead.Skipping AST parsing for JSON files will improve performance and eliminate potential edge cases if non-standard JSON (e.g., with comments) is encountered.
⚡ Proposed optimization
if (visited.has(resolved)) return; visited.add(resolved); + // JSON files have no imports to follow - skip AST parsing + if (resolved.endsWith('.json')) return; + const content = await fs.readFile(resolved, 'utf-8'); const sf = ts.createSourceFile(resolved, content, ts.ScriptTarget.Latest, true);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/dashboard/vite/utils/compiler.ts` around lines 261 - 267, The code currently treats .json files like code and calls ts.createSourceFile on them (see resolved, content, sf), which is unnecessary and slow; update the logic so that after resolving and reading the file (resolved, content) you skip AST parsing for JSON files — e.g., detect /\.json$/ on resolved (or content) and return early before calling ts.createSourceFile/searching for import/export declarations — leaving other extensions (ts/tsx/js/jsx) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/dashboard/vite/utils/compiler.ts`:
- Around line 261-267: The code currently treats .json files like code and calls
ts.createSourceFile on them (see resolved, content, sf), which is unnecessary
and slow; update the logic so that after resolving and reading the file
(resolved, content) you skip AST parsing for JSON files — e.g., detect /\.json$/
on resolved (or content) and return early before calling
ts.createSourceFile/searching for import/export declarations — leaving other
extensions (ts/tsx/js/jsx) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4dd59b3a-2682-4aff-84b3-ab61fcff0a74
📒 Files selected for processing (1)
packages/dashboard/vite/utils/compiler.ts
|
Hi, can you provide some information about the use-case that this PR is addressing? Thanks! |
I have a couple of custom plugins in my project (one for connecting to Google Sheets to upload some order data in a sheet, and one for connecting to a shipping service to get quotes for shipments), and I'm instantiating each of these plugins with some configuration objects. These objects contain information like client ids, api keys, etc, which can vary based on deployment. In my case, the simplest approach is to simply import these config objects fom json files which I can populate during the Docker build based on the target environment. I can do this via I think it's much more handy to be able to do |
|
Hi @StefanOrzu thanks for the explanation. Can you add a test to this? The suite is fixture-driven (vite/tests/*.spec.ts), so a fixtures-json-import/ with a config that imports a ./config.json + an assertion that the JSON lands in the output dir is straightforward and mirrors every other scenario. Should be a merge requirement given how cheap it is. Also, you need to sign the CLA as per this comment before the PR can be merged. Cheers! |
Description
Added the inclusion of JSON files when transpiling
vendure-config.tsand its imports during the dashboard app compilation phase. Collected JSON files aren't transpiled per se - they're just passed through as-is.Checklist
📌 Always:
👍 Most of the time:
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.