lightstep / lightstep-tracer-javascript

Lightstep distributed tracing library for Node.js and the browser
https://lightstep.com
MIT License
77 stars 66 forks source link

fix(dependencies): Update dependencies for security reasons #288

Closed Retro64 closed 2 years ago

Retro64 commented 2 years ago

This PR updates the dependencies to get rid of multiple security vulnerabilities, including async. The update for babel caused multiple adjustments in makefile and webpack config. One test was fixed, one extended by a timeout, to ensure the test is run completely. Deprecated packages like Istanbul were replaced, linting errors on existing code ignored

see #287 see #285

mwear commented 2 years ago

Hi @Retro64. Thanks for the contribution. The changes look good to me, but CI does not seem to have run on your latest commits. If you amend and repush your previous commit that might be enough to "kick" ci into action. If that doesn't work, I can take a deeper dive into why CI isn't running.

mwear commented 2 years ago

The checks are showing up now (as failed). I will dig into the CI issues and get back to you.

mwear commented 2 years ago

@Retro64 CI is fixed and the failures are legitimate. make lint appears to be failing on node 8 and 10 which is likely due to the eslint version being incompatible. I doubt there is a version of eslint that will work from from node 8 until current. Unless you know of a good workaround, I would consider removing the lint check for node 8 and 10. Node 12 is passing, and node-latest is failing due to a test timing out.

Retro64 commented 2 years ago

@mwear I already increased the timeout for one test, as it also was randomly failing. I would not wonder, if this test timeouts randomly, as this relies on the machine, the test is executed on (kind of "works on my machine" - that test worked on previous commits). Both time-outed tests share the same structure, with a loop creating spans, so I will adjust the timeout.

I guess building a workaround for old node versions might not be a good idea - node 8 and node 10 are deprecated and no longer supported - maybe newer versions should be added. I might provide a commit extending the timeout and adjust the node versions.

Retro64 commented 2 years ago

@mwear I deactivated linting as workaround on node 8 and 10, as they are not supported anymore - and linting should not change any logic. I actually would have preferred to adjust the engines in package.json to:

  "engines": {
    "node": "^12.22.0 || ^14.17.0 || >=16.0.0"
  },

But the regex in https://github.com/lightstep/lightstep-tracer-javascript/blob/master/src/imp/platform/node/platform_node.js#L29 prevented me from using caret version, so I kept to >=8.0.0 not to go deeper into the rabbit hole. There are still some deprecation warnings, when fetching the node modules, but at least the security warnings are gone.

Retro64 commented 2 years ago

@mwear I now removed Node 8 and Node 10 from CI testing (just commented for now), as both versions are no longer maintained - Node 10 leads to timeout on test (and mocha needs a newer version of Node), Node 8 indicates a Regex violation on webpack (looks like current version is no longer compatible with Node 8). Do you follow semver strictly? In this case I would need to bump the version to next major, as this needs to be considered as breaking.

mwear commented 2 years ago

@Retro64 would it be possible to just remove the linting on Node 8 and 10 and still run the build and test steps?

Retro64 commented 2 years ago

@mwear I tried as first step, but webpack broke Node 8 and Mocha Node 10

mwear commented 2 years ago

These changes have been released in the following packages:

Retro64 commented 2 years ago

@mwear thanks for your quick reaction and support!