fix(nodelss): use valid esm inject paths #369

niksy · 2024-12-06T18:30:44Z

As per exports definition in package.json, some modules don’t have fully qualified specifier so using them throws ERR_MODULE_NOT_FOUND error.

Two other things to note:

Is there a reason why we don’t use unev version of Buffer? Currently it’s expected buffer package is installed but I thinks was not intended?
I’ve tried running tests for Workerd but I’m getting following error:

Error output

Workerd: 1.20241202.0 (compatibility date: 2024-12-02)
Module fallback server listening on http://localhost:8888
Running tests...
workerd/server/server.c++:4025: debug: [ TEST ] tests:crypto_getRandomValues
workerd/server/server.c++:3052: error: Fallback service failed to fetch module; payload = 404 page not found
; spec = /?specifier=%2Funenv%2Fruntime%2Fnode%2Fcrypto%2Findex&referrer=%2Ftests&rawSpecifier=unenv%2Fruntime%2Fnode%2Fcrypto%2Findex
workerd/io/worker.c++:2067: info: uncaught exception; source = Uncaught (in promise); stack = Error: No such module "unenv/runtime/node/crypto/index".
    at Object.test (tests:11:20)
workerd/io/worker.c++:2067: info: uncaught exception; source = Uncaught (async); stack = Error: No such module "unenv/runtime/node/crypto/index".
    at Object.test (tests:11:20)
workerd/server/server.c++:4033: debug: [ FAIL ] tests:crypto_getRandomValues
workerd/server/server.c++:4025: debug: [ TEST ] tests:unenv_polyfills_buffer
workerd/server/server.c++:3052: error: Fallback service failed to fetch module; payload = 404 page not found
; spec = /?specifier=%2Funenv%2Fruntime%2Fnode%2Fbuffer%2Findex&referrer=%2Ftests&rawSpecifier=unenv%2Fruntime%2Fnode%2Fbuffer%2Findex
workerd/io/worker.c++:2067: info: uncaught exception; source = Uncaught (in promise); stack = Error: No such module "unenv/runtime/node/buffer/index".
    at Object.test (tests:39:20)
workerd/io/worker.c++:2067: info: uncaught exception; source = Uncaught (async); stack = Error: No such module "unenv/runtime/node/buffer/index".
    at Object.test (tests:39:20)
workerd/server/server.c++:4033: debug: [ FAIL ] tests:unenv_polyfills_buffer
workerd/server/server.c++:4025: debug: [ TEST ] tests:unenv_polyfills_path
workerd/server/server.c++:3052: error: Fallback service failed to fetch module; payload = 404 page not found
; spec = /?specifier=%2Funenv%2Fruntime%2Fnode%2Fpath%2Fwin32%2Findex&referrer=%2Ftests&rawSpecifier=unenv%2Fruntime%2Fnode%2Fpath%2Fwin32%2Findex
workerd/io/worker.c++:2067: info: uncaught exception; source = Uncaught (in promise); stack = Error: No such module "unenv/runtime/node/path/win32/index".
    at Object.test (tests:110:23)
workerd/io/worker.c++:2067: info: uncaught exception; source = Uncaught (async); stack = Error: No such module "unenv/runtime/node/path/win32/index".
    at Object.test (tests:110:23)
workerd/server/server.c++:4033: debug: [ FAIL ] tests:unenv_polyfills_path
workerd/server/server.c++:4025: debug: [ TEST ] tests:url_parse
workerd/server/server.c++:3052: error: Fallback service failed to fetch module; payload = 404 page not found
; spec = /?specifier=%2Funenv%2Fruntime%2Fnode%2Furl%2Findex&referrer=%2Ftests&rawSpecifier=unenv%2Fruntime%2Fnode%2Furl%2Findex
workerd/io/worker.c++:2067: info: uncaught exception; source = Uncaught (in promise); stack = Error: No such module "unenv/runtime/node/url/index".
    at Object.test (tests:24:17)
workerd/io/worker.c++:2067: info: uncaught exception; source = Uncaught (async); stack = Error: No such module "unenv/runtime/node/url/index".
    at Object.test (tests:24:17)
workerd/server/server.c++:4033: debug: [ FAIL ] tests:url_parse
workerd/server/server.c++:4025: debug: [ TEST ] tests:util_implements
workerd/server/server.c++:3052: error: Fallback service failed to fetch module; payload = 404 page not found
; spec = /?specifier=%2Funenv%2Fruntime%2Fnode%2Futil%2Findex&referrer=%2Ftests&rawSpecifier=unenv%2Fruntime%2Fnode%2Futil%2Findex
workerd/io/worker.c++:2067: info: uncaught exception; source = Uncaught (in promise); stack = Error: No such module "unenv/runtime/node/util/index".
    at Object.test (tests:100:23)
workerd/io/worker.c++:2067: info: uncaught exception; source = Uncaught (async); stack = Error: No such module "unenv/runtime/node/util/index".
    at Object.test (tests:100:23)
workerd/server/server.c++:4033: debug: [ FAIL ] tests:util_implements
workerd/server/server.c++:4025: debug: [ TEST ] tests:workerd_implements_buffer
workerd/server/server.c++:4033: debug: [ PASS ] tests:workerd_implements_buffer
workerd/server/server.c++:4025: debug: [ TEST ] tests:workerd_modules
workerd/server/server.c++:4033: debug: [ PASS ] tests:workerd_modules
workerd/server/server.c++:4025: debug: [ TEST ] tests:workerd_path
workerd/server/server.c++:4033: debug: [ PASS ] tests:workerd_path
Tests failed!

vicb · 2024-12-06T19:22:18Z

Hey @niksy could you expand on how you use/test the cloudflare preset?

The cloudflare preset match the ESBuild plugin that is used in wrangler. wrangler uses that plugin whenever you have the nodejs_compat flag in your wrangler.tom (see here).

Buffer: ["buffer", "Buffer"] means that we so set the global Buffer from node:buffer#Buffer (using the implementation from workerd).

The tests should not specify /index because $cloudflare.ts is imported over index.ts when it exists. See here. I believe the errors you attached are due to your updates - workerd tests pass on CI.

I hope this helps.

vicb

See my comments in the PR conversation.

Happy to discuss further when you explain more about your usage.

niksy · 2024-12-07T13:24:59Z

@vicb I haven’t changed anything for Cloudflare preset, this is Nodeless preset. I think this preset is not used for Cloudflare Workers so changes in this preset shouldn’t affect Cloudflare usage. Correct me if I’m wrong?

As for workerd changes, I didn’t know there is definition to use $cloudflare over index so these changes are not (and shouldn’t be) applicable here. I’ll revert them.

Nevertheless, I still get same error locally when running test even without these changes, although you’re right, it passes in the CI. Is there something that needs to be set locally for this to work (I’m not blocked by this, just trying to see if I’m using this correctly :))?

As for my following question

Is there a reason why we don’t use unev version of Buffer? Currently it’s expected buffer package is installed but I thinks was not intended?

I’ve been using this module without aliasing anything, only injecting values, so buffer was trying to find package in node_modules instead of using alias definition, but maybe this should be explicitly defined like in this PR for cases where you don’t want to alias anything.

vicb · 2024-12-08T10:43:37Z

@vicb I haven’t changed anything for Cloudflare preset, this is Nodeless preset. I think this preset is not used for Cloudflare Workers so changes in this preset shouldn’t affect Cloudflare usage. Correct me if I’m wrong?

Oops I thought you updated the cloudflare preset because the workerd test were modified, I should have paid more attention.

Cloudflare preset inherits from nodeless so it might need to get updated when nodeless is.

Workerd tests work out of the box for me.

I'll look at the changes during the week.

niksy · 2024-12-08T11:02:53Z

@vicb

Cloudflare preset inherits from nodeless so it might need to get updated when nodeless is.

From what I can see, Cloudflare preset explicitly references $cloudflare export so this shouldn’t be an issue?

pi0

Thanks for the PR. It is the right change as ESM resolution does require explicit paths (in this case, export subpaths) + cloudflare preset explicitly overrides both

vicb · 2024-12-10T10:23:32Z

LGTM, thanks @niksy and @pi0

fix(node): update import paths to fully qualified values

46058b8

niksy requested review from a team and pi0 as code owners December 6, 2024 18:30

niksy changed the title ~~fix(node): update import paths to fully qualified values~~ fix(node): update import paths to fully qualified specifiers Dec 6, 2024

vicb reviewed Dec 6, 2024

View reviewed changes

fix(node): revert workerd changes

f5afd7a

pi0 approved these changes Dec 9, 2024

View reviewed changes

vicb approved these changes Dec 10, 2024

View reviewed changes

pi0 changed the title ~~fix(node): update import paths to fully qualified specifiers~~ fix(nodelss): use valid esm inject paths Dec 10, 2024

pi0 merged commit d9d4d03 into unjs:main Dec 10, 2024
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(nodelss): use valid esm inject paths #369

fix(nodelss): use valid esm inject paths #369

niksy commented Dec 6, 2024 •

edited

Loading

vicb commented Dec 6, 2024 •

edited

Loading

vicb left a comment

niksy commented Dec 7, 2024 •

edited

Loading

vicb commented Dec 8, 2024

niksy commented Dec 8, 2024

pi0 left a comment •

edited

Loading

vicb commented Dec 10, 2024

fix(nodelss): use valid esm inject paths #369

fix(nodelss): use valid esm inject paths #369

Conversation

niksy commented Dec 6, 2024 • edited Loading

vicb commented Dec 6, 2024 • edited Loading

vicb left a comment

Choose a reason for hiding this comment

niksy commented Dec 7, 2024 • edited Loading

vicb commented Dec 8, 2024

niksy commented Dec 8, 2024

pi0 left a comment • edited Loading

Choose a reason for hiding this comment

vicb commented Dec 10, 2024

niksy commented Dec 6, 2024 •

edited

Loading

vicb commented Dec 6, 2024 •

edited

Loading

niksy commented Dec 7, 2024 •

edited

Loading

pi0 left a comment •

edited

Loading