add remaining node:module methods

[anonrig]

I've purposefully didn't add the undocumented methods (even though unenv adds them), and purposefully didn't add experimental or in active development methods.

After this lands, we can remove the unenv polyfill, but I'm not quite sure what we will do with functions that are undocumented but implemented in the polyfill (even though they throw notImplemented() errors)

cc @irvinebroque @jasnell @danlapid

[irvinebroque]

👍

[vicb]

After this lands, we can remove the unenv polyfill, but I'm not quite sure what we will do with functions that are undocumented but implemented in the polyfill (even though they throw notImplemented() errors)

I think we should not remove the unenv polyfill until we have evidence that all the functions it implements are unused. My understanding is that unenv is tree-shakeable so there is no benefit in dropping the polyfill if functions are unused. However dropping functions that are actually used will break users.

Maybe I miss something?

[jasnell]

LGTM but please add a test to ensure the expected ERR_METHOD_NOT_IMPLEMENTED errors are properly thrown.

[vicb]

That does not seem to adheres to NodeJS types?

Also can a test be added?

ref: uenv and DefinitelyTyped

[jasnell]

I personally think it's fine if the types don't match on apis we intentionally do not support and that have been deprecated in node.js. If we want to be strict about it, however, these can return an empty object instead. e.g. return {};

[anonrig]

I'll just remove extensions since it's deprecated and we're have a policy of not implementing deprecated/experimental features.

[vicb]

what about resolve?

[anonrig]

technically resolve() should throw but if it throws on a geter, it makes a lot of noise and potentially break things. that's why i converted into undefined.

[anonrig]

unenv polyfill throws an error afaict. i'm not sure what we consider a "breaking change" at this point. @jasnell

resolve: Object.assign(notImplemented("module.require.resolve"), {
      paths: notImplemented("module.require.resolve.paths"),
    }),
    ```

[jasnell]

If resolve in the polyfill exists as a function but throws an error when called, that's what we should do also.

Generally speaking, for all of these, we have to generally align with whatever the polyfill is currently doing. We can tweak from there using compat flags/dates later, but if the goal is to replace the polyfill with the built-in non-op stub, then the behavior of the built-in non-op stub should match the polyfill or we risk breaking users.

[vicb]

Sorry I might be wrong, not really have time tonight.

I think it's a breaking change because currently workerd returns a string for createRequire(root).resolve() - IIRC @jasnell implemented that.

Also @anonrig I think you are looking at the wrong file, unenv code is

export const createRequire: typeof nodeModule.createRequire = (
  file: string | URL,
) => {
  return Object.assign(workerdModule.createRequire(file), {
    resolve: Object.assign(notImplemented("module.require.resolve"), {
      paths: notImplemented("module.require.resolve.paths"),
    }),
    cache: Object.create(null),
    extensions: _extensions,
    main: undefined,
  });
};

Hope this helps

[jasnell]

Hmmm, good point about implementnig createRequire(...).resolve(...) ... we may be able to implement resolve here reasonably by having it defer to createRequire(...).resolve(...) so we don't have to leave this stubbed out and things stay more consistent....

Wait... actually did I implement createRequire(...).resolve(...) .... now I can't remember and I'm not finding it lol.. Either way, we actually ought to be able to implement resolve here correctly so we probably should.

[vicb]

Wait... actually did I implement createRequire(...).resolve(...)

I was thinking about #2636 but it looks like resolve is not implemented... my bad!