-
-
Notifications
You must be signed in to change notification settings - Fork 245
fix: add Node 18 compatibility by targeting ES2021 and node18 #2731
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
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Co-authored-by: mrlubos <[email protected]>
@copilot can you add a test that would verify this is the correct fix? It should fail if we remove the changes you've made, and it should pass if we put them back |
Co-authored-by: mrlubos <[email protected]>
Added comprehensive tests in
The tests correctly fail when these settings are removed and pass when they're in place. Verified by temporarily reverting the changes and confirming test failures. Commit: 85dfa4b |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2731 +/- ##
==========================================
- Coverage 29.31% 29.27% -0.04%
==========================================
Files 388 388
Lines 37803 37781 -22
Branches 2260 2254 -6
==========================================
- Hits 11082 11062 -20
+ Misses 26691 26689 -2
Partials 30 30
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
commit: |
Problem
The CLI was not working on Node 18, showing absolutely no output even with debug mode enabled. This was caused by the build configuration targeting ES2022, which includes JavaScript features not fully supported in Node 18 (e.g.,
Object.hasOwn
, certain array methods). Additionally, the tsup/esbuild bundler was not explicitly configured to target Node 18.Solution
This PR makes three minimal changes to ensure Node 18 compatibility:
Updated TypeScript target from ES2022 to ES2021 in both
@hey-api/openapi-ts
and@hey-api/codegen-core
packages. ES2021 is fully supported by Node 18 and provides all necessary modern JavaScript features.Added explicit
target: 'node18'
to tsup configurations to ensure esbuild compiles the code specifically for Node 18 compatibility.Improved error reporting in the CLI bin script by logging errors instead of silently exiting, which will help with debugging similar issues in the future.
Added comprehensive tests in
packages/openapi-ts/src/config/__tests__/engine.test.ts
that verify the Node 18 compatibility configuration is maintained. These tests ensure that the TypeScript target is ES2021 and the tsup target is 'node18' in both packages, and will fail if these settings are removed.Changes
packages/openapi-ts/tsconfig.base.json
: ES2022 → ES2021packages/codegen-core/tsconfig.base.json
: ES2022 → ES2021packages/openapi-ts/tsup.config.ts
: Addedtarget: 'node18'
packages/codegen-core/tsup.config.ts
: Addedtarget: 'node18'
packages/openapi-ts/bin/index.cjs
: Added error logging to catch blockpackages/openapi-ts/src/config/__tests__/engine.test.ts
: Added tests to verify Node 18 compatibility configurationTesting
--version
,--help
, and actual spec generationFixes the issue where users on Node 18 would get no output when running the CLI, as reported in the issue description.
Fixes #2730
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.