Esm import support #540
Esm import support #540
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
|
recheck |
|
The CLA check is failing due to what looks like a token expiration. I've asked on an internal channel for help figuring it out. |
sgracias1
left a comment
sgracias1
left a comment
There was a problem hiding this comment.
looks ok, can you link to the equivalent upstream otel changes
|
Upstream change PR is here: open-telemetry/opentelemetry-lambda#1574 |
| source /opt/splunk-default-config | ||
|
|
||
| export NODE_OPTIONS="${NODE_OPTIONS} --require /opt/wrapper.js" | ||
| node_version="$(node -p "process.versions.node" 2>/dev/null || echo "")" |
There was a problem hiding this comment.
This isn't really correct, as the actual node executable to run is contained somewhere in $@ - bearing in mind that the chain of shell handlers could be deeper than 1. The complexity of supporting that is not worth it and likely we will wind up simply dropping support for nodes < 20 anyway and so this code could be dropped.
There was a problem hiding this comment.
This will mean we will need to drop support for versions < 20.
We could update everything and do a release that supports the older runtimes.
Then, make another release that drops older runtime support and adds the clean mjs fix.
PS: AWS will still allow Lambda functions to be updated for nodejs version 18 till Sep 30, 2026.
https://docs.aws.amazon.com/lambda/latest/dg/lambda-runtimes.html#runtimes-deprecated
There was a problem hiding this comment.
Node.js 18 reached end of life last year. According to OTel's policy only Active or Maintenance LTS versions are supported. It would be completely fine to drop support for Node < 20 now (we just have to document it).
There was a problem hiding this comment.
Yes, I think that's the conclusion we're reaching. This will still need to wait for a regular update release before we merge it though, and that's gated on open-telemetry/opentelemetry-lambda#2122 (comment) at the moment.
| @@ -0,0 +1,101 @@ | |||
| import * as module from 'module'; | |||
There was a problem hiding this comment.
Are there any changes in this from upstream? If not, then we should copy this into our build by simply pulling it from the git submodule. If there are, then there should be a comment stating where it came from and what the tweaks are so that future maintainers can have a chance to reconcile diffs.
There was a problem hiding this comment.
Agree, they seem identical, so the submodule would work great. At the very least we should provide attribution where it's coming from.
|
@ritvick this PR contains unsigned commits. From experience I will say that the best way to approach this is to
Anything less will just be wasted effort. |
4 participants
No description provided.