Skip to content

Hey you should probably update your library #24

Closed
AmeerArsala wants to merge 5 commits into
lovell:mainfrom
AmeerArsala:main
Closed

Hey you should probably update your library #24
AmeerArsala wants to merge 5 commits into
lovell:mainfrom
AmeerArsala:main

Conversation

@AmeerArsala
Copy link
Copy Markdown

node: is now the standard and cloudflare test deployments on linux dont work without this

@lovell
Copy link
Copy Markdown
Owner

lovell commented Aug 23, 2024

Support for the optional node: prefix was added in Node.js 14.18.0.

This package is widely depended-upon and currently supports Node.js 8 onwards.

"node": ">=8"

There is no mention of child_process or fs at https://developers.cloudflare.com/workers/runtime-apis/nodejs so I can't quite understand how the proposed change will help with deployment to Cloudflare Workers. Are you able to explain this a bit more?

If this is something that is genuinely required then perhaps we could support a fallback, where the node: prefixed version of a module is required and on failure the unprefixed version is required instead?

Comment thread README.md
@@ -1,5 +1,7 @@
# detect-libc

**IF YOU ARE READING THIS, YES. THIS ONE IS THE ONE THAT FIXES THE CLOUDFLARE ISSUE. HAVE FUN TESTING IN DEV!**
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?

Comment thread package.json
{
"name": "detect-libc",
"version": "2.0.3",
"name": "@ameerarsala/detect-libc",
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?

Comment thread package.json
"repository": {
"type": "git",
"url": "git://github.com/lovell/detect-libc"
"url": "git://github.com/AmeerArsala/detect-libc"
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?

Comment thread package.json
"name": "detect-libc",
"version": "2.0.3",
"name": "@ameerarsala/detect-libc",
"version": "2.0.4-0",
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?

Comment thread package.json
"musl"
],
"author": "Lovell Fuller <npm@lovell.info>",
"author": "Ameer Arsala <ameer.arsala03@gmail.com>",
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?

@AmeerArsala
Copy link
Copy Markdown
Author

Yes I'll make those changes! My bad I just needed to get an app working as quickly as possible so i had to publish a fork on npm. Basically, if you want to use certain Node libraries with Cloudflare workers that depend on this package, it won't work. The reason is that Cloudflare workers and all products based off of that (including pages) do not run Node and instead run a stripped-down version of Node called workerd as to avoid cold starts. As a result, some of these features require external Node compatibility and you must prefix with node:

@lovell
Copy link
Copy Markdown
Owner

lovell commented Aug 24, 2024

if you want to use certain Node libraries with Cloudflare workers that depend on this package

Are you able to share more details about which library/ies you are attempting to use? I ask as many native modules use this package and, even with the proposed changes, may still be unsupported by Cloudflare Workers.

@AmeerArsala
Copy link
Copy Markdown
Author

Oh I already tried it and it worked. Libraries like LibSQL and Sharp depend on it

@lovell
Copy link
Copy Markdown
Owner

lovell commented Aug 24, 2024

In terms of Sharp and "it worked", are you talking about Cloudflare Pages or Workers? I ask as the former provides a complete Node.js environment and is known to work already (without the proposed change), the latter uses V8 isolates and is known to not work due to a lack of Web Worker support.

@AmeerArsala
Copy link
Copy Markdown
Author

I'm talking about both, though this may not apply to Pages in prod/staging (it DOES apply in dev when running with the cloudflare runtime locally). It is for sure a thing with workers though.

@lovell
Copy link
Copy Markdown
Owner

lovell commented Aug 24, 2024

I've created #25 to track future possible improvements.

Thanks for taking the time to open this PR and start the discussion. I'll close this PR for now but would be very happy to accept an updated PR that more closely matches what it proposes, if you're able.

@lovell lovell closed this Aug 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants