_detect_package_hooks throws error with "type": "module" in package.json

[rdmurphy]

Hello! I believe because the _detect_package_hooks script is ran in the same context as the package its installed in, it now errors when @zeit/git-hooks is used in a project that sets type to module in its package.json.

internal/process/esm_loader.js:74
    internalBinding('errors').triggerUncaughtException(
                              ^

TypeError [ERR_UNKNOWN_FILE_EXTENSION]: Unknown file extension "" for /Scrubbed/project/folder/.git/hooks/_detect_package_hooks
    at Loader.defaultGetFormat [as _getFormat] (internal/modules/esm/get_format.js:71:15)
    at Loader.getFormat (internal/modules/esm/loader.js:102:42)
    at Loader.getModuleJob (internal/modules/esm/loader.js:231:31)
    at async Loader.import (internal/modules/esm/loader.js:165:17)
    at async Object.loadESM (internal/process/esm_loader.js:68:5) {
  code: 'ERR_UNKNOWN_FILE_EXTENSION'
}

When I do not set type to module it runs as expected.

I was able to "fix" this locally by forcing that script to be ran as a .cjs file. This required renaming _detect_package_hooks to _detect_package_hooks.cjs and altering the hook (in this case pre-commit) to call it with that extension. Obviously not the most backward-compatible of fixes, however. (Although we're very close to v10 being EOL.)

[Qix-]

Yeah, not sure why I didn't add the extension. Does .js work? I've personally never seen .cjs, is that a new thing?

[rdmurphy]

Yeah! It's part of Node's move to supporting ESM modules while transitioning away from CommonJS. It's a bit of a mess. 😅

Once Node v10 goes end-of-life in April, all supported versions of Node (starting with Node v12 LTS) will support this new dual system. .cjs is one of a few ways users can tell Node to treat a file as CommonJS. (With .mjs doing the same for ES Modules.)

I did try to add .js, but the issue remained because due to my package.json having "type": "module" set, it assumes it is also an ES Module file and tries to load it as one. With the .cjs extension it overrides my setting and will load it as CommonJS as it expects.

I think normally this isn't a problem because Node smooths this over with files in node_modules (which have their own package.json that tell Node now to treat each of its files), but git hooks are in a tricky place because they effectively run within the scope of the project using them.

I didn't try it, but if the .git/hooks folder wouldn't get mad about having a file called package.json in it, one possible solution is putting this in there:

{
 "private": true,
 "type": "commonjs"
}

Kind of weird, but should be enough to tell Node to treat all files within that folder as CommonJS.

Another option is potentially doing a breaking change/major release to commit to the .cjs extension (or rewrite to use .mjs) so it's compatible with all environments it may get used in going forward.

[Qix-]

Using the .cjs extension wouldn't be a breaking change I don't think. Previously, Node didn't care about the extension when passing in paths from the command line, which is what we're doing now. Adding .cjs shouldn't break anyone, I don't think.

Anyway, it might take me a while to get to this; if you wanted to open a PR, I would review it ASAP and release a fix :) Thanks for opening the ticket.

[rdmurphy]

Sure! I'll try to take a swing at it this weekend.

[Qix-]

Awesome :)