💯

• We can revert the changes to telefunc/node/server/runTelefunc/assertNamingConvention.ts since a Node-like environment is required anyways.
• For path.join() we can use https://github.com/vikejs/vike/blob/b635c6af3e35dfd5bd711ac5968a748acb424de7/vike/utils/path-shim.ts.
• For path.relative() maybe we don't need a real shim: see the code below the path.relative() usage.
• The path.isAboluste() usage isn't critical and we can remove the need for path.isAbsolute(). That said, if there is a simple path.isAbsolute() shim for Windows then I'd say let's use it. (For Linux the shim is trivial.)

Pretty sure we can not revert assertNamingConvention.ts. I checked the build output of my project when building for cloudflare, everything gets compiled into one big worker.js file and it contains this:

// node_modules/.deno/[email protected]/node_modules/telefunc/dist/cjs/node/server/runTelefunc/assertNamingConvention.js
var require_assertNamingConvention = __commonJS({
  "node_modules/.deno/[email protected]/node_modules/telefunc/dist/cjs/node/server/runTelefunc/assertNamingConvention.js"(exports) {
  ...
    function assertCollocation(telefuncFilePath, appRootDir, exportValue) {
      appRootDir = appRootDir || exportValue._appRootDir || null;
      if (!appRootDir)
        return;
      let fs2;
      let path;
      const req = __require;
      try {
        fs2 = req("node:fs");
        path = req("node:path");
      } catch (_a) {
        return;
      }

I guess cloudflare is shiming node:fs just fine

The question here is: how can we bypass the static analysis of bundlers?

That's actually why the code uses const req: NodeRequire = require; req('node:path') instead of just require('node:path'), but it seems like it isn't enough. I guess the bundler does some compression first that replaces req with require and then does its static analysis to detect and bundle node:path.

One trick, for example, is to eval('req'+'uire') which is fairly reliable for bypassing the static analysis of bundlers.

Well, with the current PRs state it actually works on cloudflare, just tested it.

Cloudflare doesn't even have the notion of a filesystem so it won't work anyways. It's best we avoid node:fs and node:path/pathe to be included in edge environment such as Cloudflare workers.

everything gets compiled into one big worker.js

This is an issue that should be fixed anyways, regardless of this PR. So we might as well fix it now.

Cloudflare doesn't even have the notion of a filesystem so it won't work anyways. It's best we avoid node:fs and node:path/pathe to be included in edge environment such as Cloudflare workers.

Not sure what you mean, if I deploy my worker on cloudflare with telefunc from this PR it actually works fine.

everything gets compiled into one big worker.js

This is an issue that should be fixed anyways, regardless of this PR. So we might as well fix it now.

For context: My project is sveltekit with cloudflare adapter. It is expected that the build output is one big worker.js file because the cloudflare adapter can not output multiple workers, just one, it's a known limitation.

Cloudflare doesn't even have the notion of a filesystem so it won't work anyways. It's best we avoid node:fs and node:path/pathe to be included in edge environment such as Cloudflare workers.

Are you saying assertCollocation (which has const req: NodeRequire = require; req('node:path')) should not be bundled at all when building for edge? How would that be possible as it is part of the server runtime code (used in assertNamingConvention which is used in findTelefunction)

Are you saying assertCollocation (which has const req: NodeRequire = require; req('node:path')) should not be bundled at all when building for edge? How would that be possible as it is part of the server runtime code (used in assertNamingConvention which is used in findTelefunction)

In theory, yes, that would be the best. But in practice, as your rightly point out, to it isn't (easily) achievable.

That said, it should be possible (and in my experience relatively easy) to ensure that node:fs and node:path are never bundled (regardless of whether targeting the edge or not). The question being: how can we ensure that the dynamic imports require('node:path') and require('node:fs') are being ignored by bundlers?

The bottom line is that assertCollocation() requires a filesystem API to work, so it's useless and wasteful to bundle node:fs and node:path for any environment that doesn't seem to have a filesystem API.

The question being: how can we ensure that the dynamic imports require('node:path') and require('node:fs') are being ignored by bundlers?

Idea:

const require_ = globalThis['re' + (1 < 2 ? 'quire' : '')]
require_('node:path')

But I'm thinking that 1 < 2 may be replaced with true by clever compilers. Maybe we can replace 1 < 2 with something more difficult for compilers to analyse? Idea: new Date() > x.

Are you implying that the implementation of node:fs and node:path is being bundled?
Why would that be the case? The bundler assumes these are platform features and tries to load them during runtime. That's why we get "Error: Dynamic require of \"node:path\" is not supported" at runtime.
Am I mixing something up here?

Ah, I was wrong: your bundler actually doesn't bundle node:fs nor node:path. So we're good about that.

So if we revert the changes to telefunc/node/server/runTelefunc/assertNamingConvention.ts then it should work just fine.

If we then apply my other comments then I believe we can remove the pathe dependency.

So if we revert the changes to telefunc/node/server/runTelefunc/assertNamingConvention.ts then it should work just fine.

If we do that we import from node:path which will result in this error during runtime: "Error: Dynamic require of \"node:path\" is not supported"

I still think pathe is a good option. It's tiny and after treeshaking there wont be much left.
How do you feel about copy pasting the required pathe code to utils? Its basically one file.

So if we revert the changes to telefunc/node/server/runTelefunc/assertNamingConvention.ts then it should work just fine.

If we do that we import from node:path which will result in this error during runtime: "Error: Dynamic require of \"node:path\" is not supported"

That's surprising: the try-catch block swallows the error. Are you sure the error is shown? Anyways, if we replace node:path with pathe then we'll still have the same issue with node:fs. So I still think we should be reverting the assertNamingConvention.ts changes.

I still think pathe is a good option. It's tiny and after treeshaking there wont be much left. How do you feel about copy pasting the required pathe code to utils? Its basically one file.

I ain't categorically against it, but if we can have a couple of lines instead of using pathe then let's do that. For Open Source tools I'd rather be strict about keeping things minimal: bloat quickly adds up and it adds up for each single user.