-
Notifications
You must be signed in to change notification settings - Fork 62
feat: add nodejs 22 env #424
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
|
@sanketsudake please let me know what else is needed to get the PR merged, it should include your changes from the #423 PR |
|
@davidchase I was trying common js with ESM module, got following error. So if possible I would consider following approach, We merge your code with existing nodejs environment, mainly following files On test side we keep both common js and ES modules testcases and examples. We can add an environment variable which tells environment if function is ESM or not. Based on ESM set or not we can support both ESM and traditional functions. I am not sure if this possible, but if we can get something like this that would help current functions as well ESM functions. We can even create docker images based by setting and unsetting ESM flag. |
|
@sanketsudake so you dont want to keep separate runtimes envs? meaning those that need legacy support (cjs) would use one version of nodejs like nodejs18 and those using current features which include ESM would use nodejs22 image |
|
So nodejs 18, we can deprecate totally and support node 22 and above only as its LTS now. I am not expert in nodejs, but just trying to find middle ground so that we dont need to maintain multiple envs per language unnecessarily. |
|
@sanketsudake i have made updates to the PR with LOAD_ESM env var and added to read me how to use with env flag |
|
Can you please adjust all code under existing |
|
@sanketsudake ive moved the files into nodejs directory and removed nodejs22 directory and updated tests and added esm examples |
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 adds Node.js 22 runtime environment support with ESM (ECMAScript modules) capabilities to the existing Node.js environment, while maintaining backward compatibility with the original CommonJS-based Node.js runtime.
- Converts the existing Node.js runtime to support both ESM and CJS module loading modes
- Updates dependencies to modern versions (Express 5.1.0, Pino logging, WebSocket 8.18.3)
- Adds comprehensive test coverage for both ESM and CJS function formats
Reviewed Changes
Copilot reviewed 24 out of 28 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| nodejs/server.js | Core runtime conversion to ESM with dual CJS/ESM module loading support |
| nodejs/package.json | Package configuration updated for ESM with modern dependencies |
| nodejs/test/test_node_env.sh | Enhanced test script supporting both CJS and ESM function testing |
| nodejs/test/local_test.sh | Comprehensive local testing framework for multiple function types |
| nodejs/test/test-case-*/*.js | New ESM test cases and updated CJS test cases with proper module configuration |
| nodejs/examples/*.js | ESM example functions demonstrating Node.js 22 features |
| Makefile | Added nodejs22 test image targets |
| .github/workflows/environment.yaml | Duplicate fission dump collection step |
Files not reviewed (3)
- nodejs/package-lock.json: Language not supported
- nodejs/test/test-case-4/package-lock.json: Language not supported
- nodejs/test/test-case-8/package-lock.json: Language not supported
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
|
is there a way to auto run the actions for this PR? that way we can have a tighter feedback loop? |
The body variable needs escaped double quotes to properly pass the full string 'Its a beautiful day' through bash -c invocation
|
Please update the version here to 1.33 for the new release |
|
Thanks @davidchase, for all your hard work on the PR and for considering my suggestions. 🎉 |
Node.js 22 Runtime Environment with ESM Support
Summary
This PR introduces a separate directory for Nodejs 22 runtime env (server.js) with some dependency updates such as Pino logging instead of Morgan.
Resolves #277
New Features
Node.js 22 Environment
nodejs22/with complete environment setup"type": "module"configuration enables nativeimport/exportsyntaxESM-Ready Examples
All examples use modern ESM syntax and Node.js 22 features:
hello.jsweather.jsmulti-entry.jsbroadcast.jskubeEventsSlack.jsUsage
Environment Creation
Function Deployment (ESM)
Modern Function Syntax
Breaking Changes
None - this is purely additive. Existing
nodejs/environment remains unchanged for backward compatibility.Testing
nodejs22/test/Benefits