-
Notifications
You must be signed in to change notification settings - Fork 7
feat: align main exports across node and browser #259
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
Conversation
- add shared FilecoinPinAPI contract and mirror exports in index entries
- consolidate type exports, fix missing dist/index.js
- add isomorphic package import test covering createCarFromPath behavior
- update vitest config to run all tests via multiple projects.
- individual projects can still be ran via `npm run test:unit`,
`npm run test:integration`, `npm run test:browser`
Related to #182
SgtPooki
left a comment
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.
self review
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.
Pull request overview
This PR aligns the main exports between Node.js and browser environments by introducing a shared FilecoinPinAPI contract that both entry points satisfy. The changes consolidate type exports, add an isomorphic package import test, and update the vitest configuration to run tests across multiple projects (unit, integration, and browser).
Key Changes:
- Introduced shared
FilecoinPinAPIinterface with consistent exports acrossindex.tsandindex.browser.ts - Added multi-project vitest configuration supporting Node.js unit tests, integration tests, and browser tests
- Created isomorphic test pattern (
.iso.test.ts) that runs in both Node.js and browser environments
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| vitest.config.ts | Configures three test projects (unit, integration, browser) with appropriate include patterns for each environment |
| src/test/unit/package-import.iso.test.ts | Adds isomorphic test verifying package imports work correctly in both Node.js and browser environments |
| src/test/unit/core-data-set.test.ts | Adds biome-ignore comment for vitest mock implementation style |
| src/index.ts | New Node.js entry point exporting full API including createCarFromPath |
| src/index.browser.ts | New browser entry point with stub for Node-only createCarFromPath function |
| src/index-types.ts | Defines shared FilecoinPinAPI interface and re-exports all public types |
| package.json | Adds browser export conditions, updates test scripts, and adds browser testing dependencies |
| README.md | Documents the new test:browser npm script |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
SgtPooki
left a comment
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.
self review
|
I think we should just pick a lane on this and not make it fit everyone's preferences, that's a losing game to play. I'm fine not using default exports, they struggled out of the gate with bad and inconsistent bundler support and I think most people have just got used to not using them. I like explicitness too. |
npm run test:unit,npm run test:integration,npm run test:browsersrc/test/unit/**/*.(?iso\.)test.js,src/test/**/*.(?iso\.)test.jssrc/test/integration/**/*.test.ts(unchanged)Related to #182