Skip to content

Update NCC; pin fetch to v2#71

Merged
larsoner merged 3 commits intoscientific-python:masterfrom
nickodell:njo-update-ncc
May 20, 2025
Merged

Update NCC; pin fetch to v2#71
larsoner merged 3 commits intoscientific-python:masterfrom
nickodell:njo-update-ncc

Conversation

@nickodell
Copy link
Contributor

@nickodell nickodell commented May 19, 2025

Update NCC; pin fetch to v2

Reference Issue

#70

Reasoning behind change

I found that when using the version of NCC installed by package.json, the compiler is not capable of understanding many of the constructs within this code and its dependencies. For example, I get this error:

Module parse failed: Unexpected character '#' (13:2)
File was processed with these loaders:
 * ./node_modules/@zeit/ncc/dist/ncc/loaders/empty-loader.js
 * ./node_modules/@zeit/ncc/dist/ncc/loaders/relocate-loader.js
 * ./node_modules/@zeit/ncc/dist/ncc/loaders/shebang-loader.js
You may need an additional loader to handle the result of these loaders.
|    * @type {Map<string, import('./cache').requestResponseList}
|    */
>   #caches = new Map()

The version of NCC that the package.json refers to is no longer supported upstream. Instead, use @vercel/ncc to provide this.

I also found that it was necessary to pin node fetch to a pre v3 version. If this is not done, I get the following error when running the action:

node:internal/modules/cjs/loader:1215
  throw err;
  ^

Error: Cannot find module 'node-fetch'
Require stack:
- /home/runner/work/_actions/nickodell/circleci-artifacts-redirector-action/e396aa885116bf6964236ba34707f520d11d0034/dist/index.js
    at Module._resolveFilename (node:internal/modules/cjs/loader:1212:15)
    at Module._load (node:internal/modules/cjs/loader:1043:27)
    at Module.require (node:internal/modules/cjs/loader:1298:19)
    at require (node:internal/modules/helpers:182:18)
    at 5266 (/home/runner/work/_actions/nickodell/circleci-artifacts-redirector-action/e396aa885116bf6964236ba34707f520d11d0034/dist/index.js:29215:33)
    at __nccwpck_require__ (/home/runner/work/_actions/nickodell/circleci-artifacts-redirector-action/e396aa885116bf6964236ba34707f520d11d0034/dist/index.js:31099:43)
    at /home/runner/work/_actions/nickodell/circleci-artifacts-redirector-action/e396aa885116bf6964236ba34707f520d11d0034/dist/index.js:31127:15
    at Object.<anonymous> (/home/runner/work/_actions/nickodell/circleci-artifacts-redirector-action/e396aa885116bf6964236ba34707f520d11d0034/dist/index.js:31216:12)
    at Module._compile (node:internal/modules/cjs/loader:1529:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1613:10) {
  code: 'MODULE_NOT_FOUND',
  requireStack: [
    '/home/runner/work/_actions/nickodell/circleci-artifacts-redirector-action/e396aa885116bf6964236ba34707f520d11d0034/dist/index.js'
  ]
}

Node.js v20.19.1

As far as I can tell, this is caused by node-fetch@v3 being ESM-only, where we use a CommonJS style import.

I have not re-generated dist/index.js using these settings. I assume that since this is essentially an object file, you would prefer to have a trusted maintainer do this. If you prefer that I do this step, I can add this to the PR.

@larsoner
Copy link
Collaborator

larsoner commented May 19, 2025

I have not re-generated dist/index.js using these settings. I assume that since this is essentially an object file, you would prefer to have a trusted maintainer do this. If you prefer that I do this step, I can add this to the PR.

I'll see if I can create an autofix CI to do this, unless you know a better way!

@bsipocz bsipocz added the dependencies Pull requests that update a dependency file label May 19, 2025
@bsipocz
Copy link
Member

bsipocz commented May 19, 2025

I'll see if I can create an autofix CI to do this, unless you know a better way!

An autofix CI job would be awesome as there have been a couple of dependabot updates to the lockfile, too that have not resulted in a index.js update.

@larsoner
Copy link
Collaborator

larsoner commented May 19, 2025

@nickodell if you're motivated to try, you could add a version of this job (can be in this PR):

https://github.com/mne-tools/mne-installers/blob/main/.github/workflows/outdated.yml

If not I'll do it tomorrow hopefully

@nickodell
Copy link
Contributor Author

nickodell commented May 19, 2025

@nickodell if you're motivated to try, you could add a version of this job (can be in this PR):

I'm afraid I'm not motivated. I am already three layers deep into yak shaving, and I don't want any new yaks.

@larsoner
Copy link
Collaborator

larsoner commented May 20, 2025

Good bot, thanks for shaving that yak 🐮 (closest emoji I could think of to a yak!)

In it goes, thanks @nickodell !

@larsoner larsoner merged commit bc4ba3e into scientific-python:master May 20, 2025
3 checks passed
larsoner added a commit to matthieutrs/circleci-artifacts-redirector-action that referenced this pull request May 20, 2025
@larsoner
Merge remote-tracking branch 'upstream/master' into mm
d914c02 
* upstream/master:
  Update NCC; pin fetch to v2 (scientific-python#71)
  ENH: Add automatic rebuild (scientific-python#72)
  Build(deps): Bump undici from 5.28.5 to 5.29.0
  Making config example and narrative consistent (scientific-python#67)
@bsipocz
Copy link
Member

bsipocz commented May 20, 2025

I'm just generating noise, sorry about it. But I really like the yak analogy and will start using it myself as it's a nice counterpart for the new puppies of newly added features,

(what about 🐂🏔️?)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

dependencies Pull requests that update a dependency file

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@nickodell @larsoner @bsipocz