-
Notifications
You must be signed in to change notification settings - Fork 218
issue-245: fix CommonJS interoperability in tsc-multi and shim files #258
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?
issue-245: fix CommonJS interoperability in tsc-multi and shim files #258
Conversation
|
I wasn't quite able to get this deployed and I'm not certain this works. If would like I will close |
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.
Thank you so much for sending this. As mentioned at one of the comments, this should be good to go, but we need more tests and add automated tests under https://github.com/openai/openai-agents-js/tree/main/integration-tests; I will work on it later this week. If you're willing to help us complete these too, it'd be appreciated.
@@ -1,6 +1,6 @@ | |||
{ | |||
"targets": [ | |||
{ "extname": ".js", "module": "es2022", "moduleResolution": "node" }, | |||
{ "extname": ".js", "module": "commonjs", "moduleResolution": "node" }, |
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.
Thanks for changing this. This change looks reasonable, but we need to verify these before merging the change:
- Do more manual testing
- Add automated tests to ensure this works as expect under https://github.com/openai/openai-agents-js/tree/main/integration-tests
I will spend time on this later this week soon.
@seratch sure, feel to work on and close this pull request whenever. Unfortunately. I'm not sure if I will be able to work on in a timely fashion. This is no longer blocking me(was able to work around albeit took time) so no rush. You have my thanks |
Co-authored-by: Kazuhiro Sera <[email protected]>
hey @seratch! quick note up front: thanks to @CharlieGreenman for the import.meta shim idea, it’s folded in here. on the Lambda side: the node 22 runtime still ships without automatic module-syntax detection. i do not believe there is a public commitment from aws to change that e.g. see aws-lambda-base-images#233. so here we’re still hitting two gaps that need addressing. (1) build artifacts: we need both the ESM and CJS outputs, properly wired via the exports field, so environments like lambda and jest can safely require() the SDK without breaking compatibility for existing ESM consumers. (2) there’s runtime safety. I think the sdk's shims must avoid unguarded access to Given everything that needs to land here, I think a fresh PR is the cleanest path forward. I’m putting one together to handle both the dual-build setup and the shim guard. (and would include integration tests that cover import, require, and execution inside the relevant Lamda runtime). I'll have it wrapped up tmrw for review. |
Thanks for the inputs. I believe the direction should be good to go, but I just don't have time to do thorough testing right now. Any inputs / suggestions / contributions for this would be greatly appreciated. |
Solves:
I am building my own personal npm package. Will confirm here when works as expected (for AWS Lambda)