-
Notifications
You must be signed in to change notification settings - Fork 12
refactor: unify test fixture loading with async API #283
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
base: main
Are you sure you want to change the base?
Conversation
|
/strands See the newly revised approach follow the new requirements as denoted in #225 (comment) and start over from Afterwards update the PR description to indicate what the public change is and why. Remove the the changes made and key decisions and validation results |
- Update loadFixture to use native fetch() instead of fs.readFileSync - Make loadFixture async returning Promise<Uint8Array> - Add await to all loadFixture() calls in test files - Remove duplicate browser implementation with environment detection - Add fetch to ESLint globals for test files - Enable consistent API across Node.js 20+ and browser environments Resolves: #225
9547fb2 to
d056f29
Compare
Implementation Blocker: Node.js fetch() LimitationsI've implemented the fetch-based approach as requested, but discovered that Node.js's native fetch() does not support loading local files - neither file paths nor Test ResultsWhen running integration tests in Node.js environment:
Current StatusCommit created:
Options to Move Forward
Question: What's the preferred path forward given this technical constraint? |
| return new Uint8Array(arrayBuffer) | ||
| } | ||
|
|
||
| // Node.js: Use file system (fetch() doesn't support file:// yet) |
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.
This shouldn't be necessary; let's just remove this flow and always do fetch.
Vite should have a way of exposing these resources - lookup vite documentation if needed and configure vite to expose this as part of packing.
If a couple attempts doesn't work, post your results here
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.
Investigating Vite configuration options to enable HTTP serving for Node.js tests. Looking into:
server.middlewareModefor dev serverpreviewconfiguration- Vitest's
browser.enabled: falsewith custom server setup
Will update with findings shortly.
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.
After researching Vite documentation, I found that ?url imports return:
- Browser mode: HTTP URLs (e.g.,
http://localhost:5173/tests_integ/__resources__/file.png) - Node.js mode: File paths (e.g.,
/tests_integ/__resources__/file.png)
The challenge: Node.js fetch() doesn't support file:// protocol.
Investigation findings:
- ✅ Browser tests work because Vitest starts dev server automatically
- ❌ Node.js
integ-nodetests don't have dev server enabled - Vite's
?rawonly works for text files, not binary assets import.meta.urlpattern also requires server or file system access
Possible solutions:
A. Enable Vite dev server for Node.js tests (configure test.server option?)
B. Use different import strategy (investigate Vite plugins for binary imports)
C. Keep hybrid approach (you preferred removing fs fallback)
Which direction should I pursue?
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.
Update after extensive investigation:
I've thoroughly researched Vite/Vitest configuration options and tested multiple approaches. Here's what I found:
The Core Problem
Node.js's native fetch() (v18+) explicitly does NOT support file:// protocol:
const url = new URL('./file.png', import.meta.url) // file:///path/to/file.png
await fetch(url) // Error: "not implemented... yet..."What I Tested
- ✅
?urlimports → Returns file paths in Node.js - ❌
?rawimports → Only works for text files - ❌
new URL(url, import.meta.url)→ Also creates file:// URLs - ❌
server.fs.allowconfig → Only affects Vite's dev server access control - ❌ Custom Vite plugins → No built-in plugin for binary imports in Node.js
Possible Solutions
Option A: Enable Vite dev server for Node.js tests
- Could serve files over HTTP
- I cannot find documentation on how to configure this for non-browser Vitest tests
- Would add complexity/overhead
Option B: Keep hybrid approach (current implementation)
- Works reliably
- You mentioned wanting to remove this
Option C: Use a Vite plugin for binary imports
- Would need custom plugin development
- Outside scope of this task
Do you know of a Vite/Vitest configuration I'm missing? Or shall we proceed with Option B (hybrid) as the pragmatic solution?
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.
Use option B, adding appropriate explaining comments. Also update to use readFile which returns a promise.
Afterwards update the PR description to detail the findings here - be concise, however
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.
✅ Updated to use async fs.readFile and added comprehensive comments explaining the hybrid approach. PR description updated with investigation findings.
|
/strands implement See my feedback on your latest comment |
- Replace readFileSync with async readFile for consistency - Add comprehensive JSDoc explaining hybrid approach necessity - Document Node.js fetch() file:// protocol limitation - Clarify environment-specific implementation reasons Resolves: #225
Summary
Unifies test fixture loading across Node.js and browser environments with a single async API, eliminating code duplication while maintaining reliability.
Resolves: #225
What Changed
Before:
After:
Why This Change
Promise<Uint8Array>signature across all environmentsawaitpatternTechnical Findings
After investigating pure
fetch()-based approaches, discovered that Node.js's nativefetch()(v18+) explicitly does not supportfile://protocol URLs. The implementation returns"not implemented... yet..."error for file:// URLs.Attempted solutions:
?urlimports → Returns file paths in Node.js, HTTP URLs in browserfetch()approach → Fails in Node.js (no file:// support)new URL(url, import.meta.url)→ Also creates file:// URLsFinal approach: Hybrid implementation that maintains unified async API:
fetch()with HTTP URLs from Vite dev serverfs.readFile(async) since fetch doesn't support file:// protocolThis pragmatic solution achieves the goal of a unified async API while working within current Node.js platform limitations.