chore: bump dependencices

[mxschmitt]

• @actions/tool-cache v2.0.0 should not have any relevant breaking changes for us:
  • https://github.com/actions/toolkit/blob/main/packages/tool-cache/RELEASES.md
• @octokit/rest v19/v20 should also not affect us:
  • https://github.com/octokit/rest.js/releases/tag/v19.0.0
  • https://github.com/octokit/rest.js/releases/tag/v20.0.0
  • Except: https://github.com/octokit/octokit.js/#fetch-missing

Tested it on the branch and it worked fine.

Fixes #170

[dscho]

Sorry for taking so long. The thing I was worried about, and that took quite a while to research, is that new 37.index.js file.

tl;dr this is an intentionally-separate file, generated by ncc for a so-called "dynamic import". But it lead me to find the actual, underlying problem we need to still address.

So... what is a "dynamic import"?

Great question. I have not had much need for it in the past, only occasionally bumped into a situation where such an import is required. As Mozilla explains:

The import() syntax, commonly called dynamic import, is a function-like expression that allows loading an ECMAScript module asynchronously and dynamically into a potentially non-module environment.

That is, if you need to import functionality from a ES module (the "modern" way, normally requiring the use of import statements) from a non-ES module (AKA "CommonJS module", the "legacy" form of Javascript modules, where modules are usually imported via const ... = require(...)), you need to use the await import() form, i.e. a dynamic import. Because you cannot require() an ES module. And indeed, this happens locally here:

$ node -e 'require("node-fetch")'
node:internal/modules/cjs/loader:1307
      throw err;
      ^

Error [ERR_REQUIRE_ESM]: require() of ES Module action-tmate/node_modules/node-fetch/src/index.js from action-tmate/[eval] not supported.
Instead change the require of index.js in action-tmate/[eval] to a dynamic import() which is available in all CommonJS modules.
    at [eval]:1:1
    at Script.runInThisContext (node:vm:123:12)
    at Object.runInThisContext (node:vm:299:38)
    at [eval]-wrapper:6:22 {
  code: 'ERR_REQUIRE_ESM'
}

Node.js v18.17.1

And where does action-tmate use a dynamic import?

It doesn't. At least not directly. However, due to the upgrade to a new Octokit version, we now also need to explicitly import node-fetch, and that project uses a dynamic import.

And where exactly does it use a dynamic import, and why? Isn't node-fetch an ES module?

Well, yes. For ~7 years, node-fetch has been an ES module.

Now, the reason why this ES module cannot simply import its own multipart-parser sub-module escapes me, and the commit message of the commit that added it is too terse to be helpful in shining light on this matter, nor does the PR that introduced it.

But it is what works for them, so we'll have to live with it.

But, but, but... if node-fetch is an ES module, how can we require() it?

Well, the short answer is: we cannot, it does not work:

Error: TypeError: fetch is not a function

So here lies the rub. You will see that in this workflow run, which is essentially just the regular manual test matrix started in the bump-deps branch, all of the limit-access-to-actor: true vectors are failing, which is where that require('node-fetch') is actually executed.

So what should we do instead of require('node-fetch'), since it does not work?

The answer is... another dynamic import 😄, concretely:

request: { fetch: (await import('node-fetch')).default }

It is a bit more complicated than a regular require(), in particular because the function we seek so badly is the default export of that module, but it works.

The commit I'd like you to squash into yours looks admittedly grisly. But the horribleness is strictly limited to the lib/ directory, which is the generated output of ncc (which we must trust to do a good job).

[dscho]

@mxschmitt between all those notes I needed to sort and organize into a coherent review, I totally forgot to show my gratitude for taking on this huge task. My apologies. And: thank you!

Also:

request: { fetch: (await import('node-fetch')).default }

I just realized that you could also reinstate import fetch from "node-fetch" (which I just noticed was reverted yesterday, and I assume the reason for it was that it was not mocked, but that should be easy via jest.mock('node-fetch'), no?).

[mxschmitt]

Hi,

yeah the reason why I changed it to a CJS import, was the following:

action-tmate on  bump-deps [!?] via  v18.17.1 
❯ npm run test

> action-tmate@0.0.0 test
> GITHUB_EVENT_PATH= jest

 FAIL  src/index.test.js
  ● Test suite failed to run

    Jest encountered an unexpected token

    Jest failed to parse a file. This happens e.g. when your code or its dependencies use non-standard JavaScript syntax, or when Jest is not configured to support such syntax.

    Out of the box Jest supports Babel, which will be used to transform your files into valid JS based on your Babel configuration.

    By default "node_modules" folder is ignored by transformers.

    Here's what you can do:
     • If you are trying to use ECMAScript Modules, see https://jestjs.io/docs/ecmascript-modules for how to enable it.
     • If you are trying to use TypeScript, see https://jestjs.io/docs/getting-started#using-typescript
     • To have some of your "node_modules" files transformed, you can specify a custom "transformIgnorePatterns" in your config.
     • If you need a custom transformation specify a "transform" option in your config.
     • If you simply want to mock your non-JS modules (e.g. binary assets) you can stub them out with the "moduleNameMapper" config option.

    You'll find more details and examples of these config options in the docs:
    https://jestjs.io/docs/configuration
    For information about custom transformations, see:
    https://jestjs.io/docs/code-transformation

    Details:

    /Users/maxschmitt/Developer/action-tmate/node_modules/node-fetch/src/index.js:9
    import http from 'node:http';
    ^^^^^^

    SyntaxError: Cannot use import statement outside a module

       7 | import * as tc from "@actions/tool-cache"
       8 | import { Octokit } from "@octokit/rest"
    >  9 | import fetch from "node-fetch"
         | ^
      10 |
      11 | import { execShellCommand, getValidatedInput, getLinuxDistro, useSudoPrefix } from "./helpers"
      12 |

      at Runtime.createScriptFromCode (node_modules/jest-runtime/build/index.js:1505:14)
      at Object.require (src/index.js:9:1)
      at Object.require (src/index.test.js:28:1)

----------|---------|----------|---------|---------|-------------------
File      | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s 
----------|---------|----------|---------|---------|-------------------
All files |       0 |        0 |       0 |       0 |                   
----------|---------|----------|---------|---------|-------------------
Test Suites: 1 failed, 1 total
Tests:       0 total
Snapshots:   0 total
Time:        2.354 s
Ran all test suites.

action-tmate on  bump-deps [!?] via  v18.17.1 took 5s 

Fixed it by adding babel-jest as a layer which transpiles everything to CJS. Ideally we should require the lib files instead of the src files inside the tests, but probably not a big deal here.

[dscho]

@mxschmitt unfortunately I get that TypeError: fetch is not a function problem again: https://github.com/mxschmitt/action-tmate/actions/runs/6083513664/job/16503525646#step:5:18

[dscho]

So this now supersedes #173...

@mxschmitt I won't have time to tag and release it today, feel free to do that if you have the time.