Skip to content
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

Cannot find module 'node:http` on AWS Lambda v14 #1367

Closed
pawelgrzybek opened this issue Nov 9, 2021 · 34 comments
Closed

Cannot find module 'node:http` on AWS Lambda v14 #1367

pawelgrzybek opened this issue Nov 9, 2021 · 34 comments
Labels

Comments

@pawelgrzybek
Copy link

Upgrade from version 3.0 to 3.1 produces a Runtime.ImportModuleError: Error: Cannot find module 'node:http on AWS Lambda runtime.

Screenshots

Screenshot 2021-11-09 at 07 32 57

Your Environment

software version
node-fetch 3.1
node 14

Additional context

Looks like node: prefix for imports added as part of #1346 causes this issue. I am happy to submit MR with a revert of this one, but I need to understand the reason why this prefix has been added here on the first place. Can you suport me on this one please @dnalborczyk ?

@jimmywarting
Copy link
Collaborator

https://nodejs.org/dist/latest-v17.x/docs/api/esm.html#node-imports

We are targeting node v12.20 and above. and node: prefix got supported in 12.20 for esm syntax, but not for require syntax until v14.18

@pawelgrzybek
Copy link
Author

pawelgrzybek commented Nov 9, 2021

@jimmywarting In this case it looks like it is going to break for all CJS users between versions 12.20 (your target version) and 14.8. In my case looks like AWS runtime is <14.18.

@martin-dimitrov13
Copy link

martin-dimitrov13 commented Nov 9, 2021

Same here, locking at 3.0.0 until a fix is deployed.

@pawelgrzybek
Copy link
Author

@martin-dimitrov13 thats done

#1368

@jimmywarting
Copy link
Collaborator

Are you running some job to compile the source to cjs?

@pawelgrzybek
Copy link
Author

@jimmywarting, the build part is happening on AWS side. I use CDK framework and I send .ts files to AWS. I believe it is compiled using esbuild under the hood (not 100% confidence about this one).

https://aws.amazon.com/cdk/

@LinusU
Copy link
Member

LinusU commented Nov 9, 2021

In this case it looks like it is going to break for all CJS users between versions 12.20 (your target version) and 14.8. In my case looks like AWS runtime is <14.18.

Hmm, but version 3.x of node-fetch is ESM only so we don't actually support CJS at all.

It seems like in this case there is a build step that turns import into require, but still keeps the node: prefix. I was unable to find what transpiling process that AWS CDK uses from a quick glance...

@pawelgrzybek
Copy link
Author

Does node-fetch actually take any advantage of node: prefix at the moment? If no I personally see no reason of keeping it just for the sake of new syntax.

@dnalborczyk
Copy link
Contributor

dnalborczyk commented Nov 9, 2021

I need to understand the reason why this prefix has been added here on the first place. Can you suport me on this one please @dnalborczyk ?

it's to distinguish between userland modules and node.js built-in modules. it serves as a namespace.

@dnalborczyk
Copy link
Contributor

@jimmywarting In this case it looks like it is going to break for all CJS users

this is an esm module, I would argue it's already "broken" for cjs users.

@pawelgrzybek
Copy link
Author

@dnalborczyk thank you for explanation. Today I learned about node: prefix.

I will leave it for people smarter than me to decide if it is useful in context for this project or not. I just wanted to report because it broke my project and can potentially affect many more.

Wait a sec, because the 3.0 version worked just fine and 3.1 collapsed on some runtimes, shouldn't that be a major release?

@dnalborczyk
Copy link
Contributor

dnalborczyk commented Nov 9, 2021

the build part is happening on AWS side. I use CDK framework and I send .ts files to AWS.

very unlikely. I'm fairly certain the build is happening locally on the dev side.

I believe it is compiled using esbuild under the hood (not 100% confidence about this one).

aws.amazon.com/cdk

you are very likely down leveling to commonjs, you should check your tsconfig.json module setting. aws lambda does not support esm modules yet.

you can see it in the stacktrace, Userfunction.js throwing: https://github.com/aws/aws-lambda-nodejs-runtime-interface-client/blob/c31c41ffe5f2f03ae9e8589b96f3b005e2bb8a4a/src/utils/UserFunction.ts#L75 handlers are being cjs required.

@pawelgrzybek
Copy link
Author

very unlikely. I'm fairly certain the build is happening locally on the dev side.

@dnalborczyk you are right. It is part of a deployment and it is compiled locally using esbuild.

@dnalborczyk
Copy link
Contributor

Wait a sec, because the 3.0 version worked just fine and 3.1 collapsed on some runtimes, shouldn't that be a major release?

good question, it depends on the point of view. let's say you use a transpiler (e.g. babel or typescript) in your build step, which down levels also node_modules dependencies. if node-fetch would refactor to use a newer syntax which is supported by all supported node-versions, but let's say for the sake of the argument not by Babel, it would break your build step in the middle of a minor or patch version. would that justify a major version? it's hard to foresee all permutations users can use a module.

@LinusU
Copy link
Member

LinusU commented Nov 9, 2021

Wait a sec, because the 3.0 version worked just fine and 3.1 collapsed on some runtimes, shouldn't that be a major release?

With semver a major change is when you have breakage in your public api. Since CJS wasn't supported at all, I don't think that this should be a major bump.

E.g. someone could depend on the exact byte size of the package, and then we add a few lines of code and their project is now to big for the Lambda size restriction. Is that a breaking change for node-fetch?

I will leave it for people smarter than me to decide if it is useful in context for this project or not. I just wanted to report because it broke my project and can potentially affect many more.

I'm personally not against removing the prefix, but since we have no tests that covers this, and since there is no documentation that it should work, I think that it will probably end up being broken again down the line...

@jimmywarting
Copy link
Collaborator

jimmywarting commented Nov 9, 2021

I'm personally not against removing the prefix,

We have already had requests like the below that ask for node: prefix to separate out what is a node module and what is a npm package

we can't just keep jumping and toggle between using it and not using it.


somewhere/somehow i think that bundlers that downlevel code into commonjs should also strip away the node: part if it's targeting v14.17 and below

@mattfysh
Copy link

mattfysh commented Nov 9, 2021

I'm using esbuild to bundle before deploying to lambda where node-fetch is now broken when using 3.1.0

Here is the relevant esbuild issue where they talk about compiling ESM import 'node:*' into calls CJS require('node:*): evanw/esbuild#1466

I'll raise an issue there to see if they can check the target environment and if below v14.18 strip the node: prefix from the require call

Does anyone know how to find out what exact version of node the AWS 14.x Lambda's are running?

@dnalborczyk
Copy link
Contributor

Does anyone know how to find out what exact version of node the AWS 14.x Lambda's are running?

you can find out running process.versions.node. anyhow, as of today: v12.22.4 and v14.17.4

@berenddeboer
Copy link

we can't just keep jumping and toggle between using it and not using it.

Although true, "node:http" is breaking builds everywhere, and not supported until you have very modern node versions. I suggest wait till the next node LTS.

@mwgamble
Copy link

A change like this should have been made in a major version bump IMO

@jimmywarting
Copy link
Collaborator

jimmywarting commented Nov 12, 2021

A change like this should have been made in a major version bump IMO

it kind of did, yet it didn't for some... We released 3.0.0 with a node-engine field set to 12.20 where node: prefix is supported (but only for import/esm), cjs supported node prefix later in 14.18

"node": "^12.20.0 || ^14.13.1 || >=16.0.0"

We just didn't account for backend users to transpile it down to commonjs.

I guess a solution for now could be to do what is recommended in: #1279 (comment) ...to use lazy import()

or do what was suggested in ESBuild 👇

@LinusU
Copy link
Member

LinusU commented Nov 12, 2021

I very simple solution for ESBuild have been suggested here: evanw/esbuild#1760 (comment)

Is there anyone having problems with this outside of ESBuild?

@berenddeboer
Copy link

very simple solution for ESBuild have been suggested here

100s of 1000s of programmers will have to Google this error, come here, and apply the patch. That seems to be a colossal waste of time.

And esbuild is often used implicitly. I.e. if you use CDK with the NodeJsFunction, it calls esbuild. People might not even know that happens.

@basickarl
Copy link

I am in agreeance that this should of been a major version increase. You just simply do not touch this kind of stuff in minor version bumps.

@jimmywarting
Copy link
Collaborator

good news for those who use esbuild, a PR was merged to strip away node: protocol
(updating esbuild might help - if they have released a new version)

@jimmywarting
Copy link
Collaborator

looks like jest have also added some support for node protocol a while back: jestjs/jest#11331

@ChromeQ
Copy link

ChromeQ commented Nov 24, 2021

I'm running into this with node v16.9.1
Although I am using create-react-app with custom babel the problem occurs when running jest. I ran into this issue and fix since node-fetch v3 and so had to ignore add transformIgnorePatterns: ['node_modules/(?!(fetch-blob|node-fetch)/)'].

Worked fine with [email protected] but breaks on v3.1 with

ENOENT: no such file or directory, open 'node:http'

@ChromeQ
Copy link

ChromeQ commented Nov 24, 2021

My solution for now in case it helps anyone; I was already using babel-plugin-module-resolver in my babel.config.js plugins so I've just extended its use to cover tests:

// babel.config.js
module.exports = (api) => {
    const env = api.env();
    // ...
    if (env === 'test') {
        plugins.push([
            'babel-plugin-module-resolver', {
                alias: {
                    '^node:(.*)$': '\\1',
                },
        }]);
    }
    // ...
}

This will replace all imports that match node:(.*) with just the name of the module. This seems to work for now.

But ideally #1368 will be merged and this won't be needed, or the jest fixes jestjs/jest#11331 added to jest v27.1 will make it's way into the next version of create-react-app, looks close to v5 milestone

@dnalborczyk
Copy link
Contributor

the latest node.js version on aws is now 14.18.1 and therefore supports the node: prefix for commonjs.

@LinusU
Copy link
Member

LinusU commented Dec 2, 2021

That's amazing!

Since Lambda updates to this automatically (the only available runtime for Node.js 14 is nodejs14.x), and since this is also fixed upstream in ESBuild, I think that we can close this issue 🎉

@ghengeveld
Copy link

ghengeveld commented Jan 28, 2022

This broke our smoke test on GitHub Actions with their Node 12 runtime. We transpile our code using Webpack. Any suggestions to remedy the situation? At the moment I'm unable to upgrade node-fetch beyond v3.0. Upgrading Node is not an option because this smoke test intentionally runs against Node 12 in order to guarantee it works in legacy environments.

Honest question: is the node: prefix required on any of the current Node.js versions? Why not drop the prefix until it becomes a requirement? At that point Node 12 will hopefully no longer be a version that needs to be supported and so it would not be an issue anymore.

@ascorbic
Copy link

Your action is running 12.22, which should support node: imports so I'm not sure what's wrong there.

@jimmywarting
Copy link
Collaborator

jimmywarting commented Jan 28, 2022

Your action is running 12.22, which should support node: imports so I'm not sure what's wrong there.

node: protocol is only available in type=modules or with import()s
it was later added to require() in v14.18.0

Version Changes
v16.0.0, v14.18.0 Added node: import support to require(...).
v14.13.1, v12.20.0 Added in: v14.13.1, v12.20.0

the issue is that they use webpack to probably pack everything to a cjs module
node-fetch wasn't built to deal bundlers in mind (as it's a server side tool - and you should just import it anyway)
as such we tough it would be okey to use esm features like node: protocol - cuz we code things with ESM in mind.

really the only possible way would be to compile to esm and if you have to ship your application to AWS lamba as a cjs module then you should load it using async import. import('./esm-app.js') as such node: protocols will start to work.

Why not drop the prefix until it becomes a requirement?

We have already had this requirement in fetch-blob where we try to load stream/web but someone reported that that module wasn't found - it was treated as a npm package and not a NodeJS built-in module. but what they did notice was that if things started with node: then they could for sure figure out that it was a built-in module and the bundler would not try to get something that comes from npm - as such we started to use node: protocol everywhere

dropping it now would only break it for somebody else...

@jimmywarting
Copy link
Collaborator

i'm just guessing now but i think that if you update webpack then it can strip away that node prefix for you
seems to be some new-ish pr that deals with node prefix
webpack/webpack#12693

alternative is that you have some webpack plugin that resolves modules manually

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.