upgradejs / depngn

A CLI tool to find out if your dependencies support a given version of node.
MIT License
103 stars 4 forks source link

[Bug]: depRange.replaceAll is not a function #32

Closed ThisIsMissEm closed 1 year ago

ThisIsMissEm commented 1 year ago

Expected Behavior

I was expecting a failing report (this is a node.js v14.20.0 project with some really bad/messy dependencies)

Actual Behavior

The following output:

$ npx depngn 18
Error: TypeError: depRange.replaceAll is not a function
    at /Users/redacted/.npm/_npx/80daaf8cfdca6619/node_modules/depngn/dist/cli.cjs:7609:27
    at step (/Users/redacted/.npm/_npx/80daaf8cfdca6619/node_modules/depngn/dist/cli.cjs:69:23)
    at Object.throw (/Users/redacted/.npm/_npx/80daaf8cfdca6619/node_modules/depngn/dist/cli.cjs:50:53)
    at rejected (/Users/redacted/.npm/_npx/80daaf8cfdca6619/node_modules/depngn/dist/cli.cjs:41:65)
    at processTicksAndRejections (internal/process/task_queues.js:95:5)

Looks like maybe one of the dependencies may be doing something weird with the engines field or something?

Steps to Reproduce

  1. Use node.js 14.x (yes, I know, it's well and truly out of date)
  2. Run depngn: npx depngn 18 on a project
  3. The actual behaviour is observed

If you want a repository you can try this in, then see: https://github.com/inrupt/pod-browser

Additional Information

I tried to add debug logging around the depRange line, and the only output it gave was { depRange: '>=6.9.0' } before failing. I tried doing a typeof on depRange and it said "string", so I did a typeof depRange.replaceAll and it said undefined.

It looks like in node.js 14.x, String.prototype.replaceAll does not yet exist (it's a newer language feature):

Welcome to Node.js v14.20.0.
Type ".help" for more information.
> String.prototype.replaceAll
undefined
>

So if you want to support Node.js 14.x then you'll need to add a polyfill in.

What package manager are you seeing the problem with?

npm

Relevant log output

No response

Code of Conduct

ThisIsMissEm commented 1 year ago

I added in the polyfill from https://vanillajstoolkit.com/polyfills/stringreplaceall/ to .npm/_npx/80daaf8cfdca6619/node_modules/depngn/dist/cli.cjs and it now works in node.js 14.x, but would still require a patch from your side.

Looks like you're intending to support Node.js >= 10.0.0:

  "engines": {
    "node": ">=10.0.0"
  },
arielj commented 1 year ago

Maybe we can use a global regexp to not need a polyfill? like replace(/something/g, ...), that g flag should work like a replaceAll

ThisIsMissEm commented 1 year ago

That's basically what the polyfill does anyway!