-
Notifications
You must be signed in to change notification settings - Fork 41
Remove Node version manager configs (v8) #1332
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: version-8
Are you sure you want to change the base?
Conversation
Drop support for Node 16, which is long past EOL. This will be merged into the `version-8` branch. >[!WARNING] > >Node 18 is where we're moving to, but that will also be EOL in April, 2025. Does this require changes to the WorkOS Docs? E.g. the [API Reference](https://workos.com/docs/reference) or code snippets need updates. ``` [ ] Yes ``` If yes, link a related docs PR and add a docs maintainer as a reviewer. Their approval is required.
>[!Note] > >This PR is against the `version-8` branch. ## Description TSLint has been EOL for several years. This replaces it with typeScript-eslint and provides a comparable configuration for minimal changes to the code. ## Documentation Does this require changes to the WorkOS Docs? E.g. the [API Reference](https://workos.com/docs/reference) or code snippets need updates. ``` [ ] Yes ``` If yes, link a related docs PR and add a docs maintainer as a reviewer. Their approval is required.
## Description ## Documentation Does this require changes to the WorkOS Docs? E.g. the [API Reference](https://workos.com/docs/reference) or code snippets need updates. ``` [ ] Yes ``` If yes, link a related docs PR and add a docs maintainer as a reviewer. Their approval is required.
>[!note] >This is against the `version-8` branch, which drops support for Node 16, allowing for this update to happen. Update to latest version of iron-session. Does this require changes to the WorkOS Docs? E.g. the [API Reference](https://workos.com/docs/reference) or code snippets need updates. ``` [ ] Yes ``` If yes, link a related docs PR and add a docs maintainer as a reviewer. Their approval is required.
>[!note] >This change is against the `version-8` branch. ## Description Upgrade to Prettier v3 and commit minimal changes resulting from that. ## Documentation Does this require changes to the WorkOS Docs? E.g. the [API Reference](https://workos.com/docs/reference) or code snippets need updates. ``` [ ] Yes ``` If yes, link a related docs PR and add a docs maintainer as a reviewer. Their approval is required.
>[!note] >This change is against the `version-8` branch. ## Description Update to latest of the following dev dependencies - TypeScript - ts-jest - Jest - Supertest ## Documentation Does this require changes to the WorkOS Docs? E.g. the [API Reference](https://workos.com/docs/reference) or code snippets need updates. ``` [ ] Yes ``` If yes, link a related docs PR and add a docs maintainer as a reviewer. Their approval is required.
Adds tests and updates typing for pagination utils.
…rt resolution across all target runtimes (#1301) ## Description This PR implements comprehensive universal runtime compatibility for the WorkOS Node.js SDK v8, addressing critical module resolution issues that prevented the package from working correctly across different JavaScript runtimes. ### Problem Statement The current v8 build configuration with `tsup` and `bundle: false` created incompatible import patterns: - **CJS files** attempted to `require()` ESM files (`.js` extension) - **ESM files** imported without explicit file extensions, breaking strict runtimes like Deno - **Standalone Node.js** execution failed due to cross-format imports - **Edge Runtime** and **Cloudflare Workers** compatibility was broken ### Solution Overview **Phase 1: Import Path Rewriting** ✅ - Implemented custom `fixImportsPlugin` to rewrite import paths during build - ESM builds now use explicit `.js` extensions for all relative imports - CJS builds use `.cjs` extensions for internal module references - Maintains bundler compatibility while fixing standalone runtime execution **Phase 2: Enhanced Export Mapping** ✅ - Added runtime-specific export conditions for optimal module resolution - Separate TypeScript type paths for CJS (`.d.cts`) and ESM (`.d.ts`) - Dedicated worker builds for edge runtimes (`workerd`, `edge-light`) - Performance-ordered conditions for faster resolution **Phase 3: Comprehensive Testing Infrastructure** ✅ - Created ecosystem compatibility test suite covering 6 runtime scenarios - Manual validation commands for immediate testing - Automated CI/CD workflow with matrix strategy testing Node.js 18/20, Deno, and Bun - Fail-fast smoke tests for quick compatibility validation ### Runtime Compatibility Matrix | Runtime | CJS Build | ESM Build | Status | Notes | |---------|-----------|-----------|---------|-------| | **Next.js/webpack** | ✅ | ✅ | Working | Bundler handles resolution | | **Node.js standalone** | ✅ | ✅ | **FIXED** | Import rewriting resolved cross-format issues | | **Deno** | N/A | ✅ | **FIXED** | Explicit .js extensions now provided | | **Bun** | ✅ | ✅ | Working | Enhanced with runtime-specific exports | | **Edge Runtime** | ✅ | ✅ | **FIXED** | Dedicated worker builds | | **Cloudflare Workers** | ✅ | ✅ | **FIXED** | Optimized for workerd environment | ### Key Technical Changes 1. **Custom Import Rewriter Plugin** (`scripts/fix-imports-plugin.ts`) - Transforms relative imports to include appropriate file extensions - Format-aware: `.js` for ESM, `.cjs` for CommonJS - Preserves external module imports unchanged 2. **Enhanced Package.json Exports** ```json { "exports": { ".": { "types": { "require": "./lib/cjs/index.d.cts", "import": "./lib/esm/index.d.ts" }, "workerd": "./lib/esm/index.worker.js", "edge-light": "./lib/esm/index.worker.js", "deno": "./lib/esm/index.js", "bun": { "import": "./lib/esm/index.js", "require": "./lib/cjs/index.cjs" }, "node": { "import": "./lib/esm/index.js", "require": "./lib/cjs/index.cjs" }, "import": "./lib/esm/index.js", "require": "./lib/cjs/index.cjs", "default": "./lib/esm/index.js" } } } ``` 3. **CI/CD Integration** (`.github/workflows/runtime-tests.yml`) - Matrix strategy testing across Node.js versions and alternative runtimes - Automated validation on every PR and push - Quick smoke tests for immediate feedback ### Testing Results All 6 runtime scenarios now pass: - ✅ Node.js CJS: `WorkOSNode` - ✅ Node.js ESM: `WorkOSNode` - ✅ Deno: `WorkOSNode` - ✅ Bun CJS: `WorkOSNode` - ✅ Bun ESM: `WorkOSNode` - ✅ Worker: Module resolution successful ### Verification Commands ```bash # Node.js CJS node -e "console.log('CJS:', require('./lib/cjs/index.cjs').WorkOS.name)" # Node.js ESM node -e "import('./lib/esm/index.js').then(m => console.log('ESM:', m.WorkOS.name))" # Deno deno eval "import('./lib/esm/index.js').then(m => console.log('Deno:', m.WorkOS.name))" # Bun bun -e "console.log('Bun:', require('./lib/cjs/index.cjs').WorkOS.name)" # Comprehensive test suite pnpm check:runtimes ``` ### Backward Compatibility - ✅ **No breaking changes** to existing API surface - ✅ **Bundler compatibility** maintained (Next.js, webpack, Vite, etc.) - ✅ **Tree-shaking** still works properly - ✅ **Package size** unchanged (unbundled builds) - ✅ **TypeScript support** enhanced with improved type resolution ### Risk Mitigation - Extensive testing across all target runtimes before merge - Automated CI validation prevents regressions - Rollback plan available (revert to `bundle: true` if needed) - Industry-standard patterns based on OpenAI SDK, Drizzle ORM practices ## Documentation ``` [ ] Yes ``` This change improves internal module resolution and doesn't affect the public API, so no documentation updates are required.
Type constructEvent payload parameter as Record<string, unknown>.
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.
Greptile Summary
This PR removes Node.js version manager configuration files from the v8 branch, mirroring changes made in PR #1331 for the main branch. The changes include:
- Removal of
.node-version
file: This file previously containedv18.20.7
and was used by various Node version managers (fnm, n, asdf) to automatically switch to the correct Node.js version - Modification of
package.json
: The Volta configuration has been simplified by removing the Node.js version pin (19.9.0
) while keeping the Yarn version constraint (1.22.19
)
The project maintains Node.js version requirements through the engines
field in package.json
which specifies "node": ">=18"
. This change is part of a cleanup effort to reduce configuration file duplication and simplify the development environment setup. Rather than maintaining multiple version management files (.nvmrc
, .node-version
, and Volta configs), the project now relies primarily on the universal package.json
engines field for version constraints.
This change aligns the v8 branch configuration with the main branch and reduces potential conflicts between different Node version management tools while still enforcing minimum Node.js version requirements during package installation.
Confidence score: 4/5
- This PR is safe to merge with minimal risk as it only affects development environment configuration
- Score reflects that while the changes are low-risk, removing version manager files may impact some developers' workflows who rely on automatic Node version switching
- No files require special attention as these are straightforward configuration changes
2 files reviewed, no comments
d4f3297
to
d477188
Compare
Same as #1331 but for the v8 branch.