it looks like removeNodeProtocol doesn't remove the node protocol import created by rolldown:runtime after testing manually by changing the config, breaking compatibility. let's hope tsdown fixes this quickly 🙏 i've opened a tsdown bug report rolldown/tsdown#276

however, fdir is currently compatible with at least node 12 (and probably even node 10 but i'm not sure i haven't checked), and this is currently breaking that compatibility due an import at the top of the file with the node: protocol

However, createRequire was added in node@12.2.0. Even if the node: protocol is removed, there would still be an API compatibility issue for versions below Node 12.2.0

Regarding node compat in general: There is no note in the docs (besides "any version") nor the engine field. This would be good to document 🤔

@TheAlexLichter tsdown 0.12.5 seems to have fixed the node: protocol issue, can you update the pr to use it?

@SuperchupuDev Done!

wait, as much as i love pnpm you should probably remove the pnpm lockfile added in your last commit (i suppose by accident?) since this repo is configured to use npm

wait, as much as i love pnpm you should probably remove the pnpm lockfile added in your last commit (i suppose by accident?) since this repo is configured to use npm

Sorry, yes by accident. Muscle memory was too strong here :D

After building with tsdown dual format, the import time for fdir decreased from 9~10 ms to ~6 ms.

@thecodrr do you mind taking a look at this and releasing a new version afterwards? (which would include a fix in main that hasn't been released yet) 🙏

@thecodrr Hey! 👋 Is there anything missing or something that is blocking the PR?

it looks like tsdown's usage of createRequire won't affect compatibility, as the very old node versions that support esm but not createRequire (node 12.0 and 12.1, back when esm was flagged) will use the cjs version anyways (the exports field wasn't a thing back then) 👍

unrelated, do you mind setting the engines.node field to >=12.0.0 instead of using the tsdown target field? tsdown can read from it plus it also resolves #116

i've just locally tested node 10-12 in main and 12 is definitely the minimum version as everything else below breaks due to class initialization stuff

This is ready for merge. I think the engines.node field should be set in a separate PR.

I'm seeing an issue when fdir@6.5.0 is bundled using esbuild.

Generated code:

// ../../../../node_modules/.pnpm/fdir@6.5.0_picomatch@4.0.3/node_modules/fdir/dist/index.mjs
var import_module = require("module");
var import_path2 = require("path");
var nativeFs = __toESM(require("fs"), 1);
var import_meta2 = {};
var __require = /* @__PURE__ */ (0, import_module.createRequire)(import_meta2.url);
function cleanPath(path14) {
  let normalized = (0, import_path2.normalize)(path14);
  if (normalized.length > 1 && normalized[normalized.length - 1] === import_path2.sep) normalized = normalized.substring(0, normalized.length - 1);
  return normalized;
}

error:

node:internal/modules/cjs/loader:1895
    throw new ERR_INVALID_ARG_VALUE('filename', filename, createRequireError);
          ^

TypeError [ERR_INVALID_ARG_VALUE]: The argument 'filename' must be a file URL object, file URL string, or absolute path string. Received undefined
    at createRequire (node:internal/modules/cjs/loader:1895:11)
    at Object.<anonymous> (/Users/jason/projects/cspell/test-packages/cspell/test-cspell-esbuild-cjs/bin/dist/index.cjs:42093:65)
    at Module._compile (node:internal/modules/cjs/loader:1692:14)
    at Object..js (node:internal/modules/cjs/loader:1824:10)
    at Module.load (node:internal/modules/cjs/loader:1427:32)
    at Module._load (node:internal/modules/cjs/loader:1250:12)
    at TracingChannel.traceSync (node:diagnostics_channel:322:14)
    at wrapModuleLoad (node:internal/modules/cjs/loader:235:24)
    at Module.require (node:internal/modules/cjs/loader:1449:12)
    at require (node:internal/modules/helpers:135:16) {
  code: 'ERR_INVALID_ARG_VALUE'
}