actions / toolkit

The GitHub ToolKit for developing GitHub Actions.
https://github.com/features/actions
MIT License
5.02k stars 1.45k forks source link

Remove dependency on `uuid` package #1824

Closed joshmgross closed 1 month ago

joshmgross commented 2 months ago

A common pattern in the toolkit is to create temporary file or directory within the existing temp directory using a random UUID.

Versions of the uuid package below v7 are deprecated, so we shouldn't depend on them. Additionally, Node has a built-in crypto.randomUUID() method that can be used to generate UUIDs without the need for an external package. This function was introduced in versions of Node 14 and 15, so we should be safe to use it for actions that depend on both Node 16 and 20 (which are our only supported versions at this time).

I also went ahead and updated @actions/core, even though it was using a non-deprecated version of the uuid package.

Since these use cases is purely for temporary files, I don't think we strictly need a cryptographically secure UUID but that's an added bonus of using the built-in Node function.

Packages shouldn't be depending on the exact format of this temp file/directory, but even if they were this should be a compatible as we're still generating a version 4 UUID

Generates a random RFC 4122 version 4 UUID. The UUID is generated using a cryptographic pseudorandom number generator.

There are a couple other packages within the toolkit that have an indirect dependency on uuid through @actions/core, those can be updated as well once we release this new version of @actions/core.

MikeMcC399 commented 1 month ago

@joshmgross

Are you going to be able to progress this PR soon? It looks like you are just waiting for your team to review.

gfteix commented 1 month ago

As the crypto global package is being used without an import/require statement, if the client is using a node version < 19 (when crypto became a global package in node.js https://nodejs.org/api/globals.html#crypto_1) the action that uses the toolkit will fail with a crypto is not defined error. Is this expected?

MarioUhrikTakeda commented 1 month ago

This seems to have caused https://github.com/actions/toolkit/issues/1841 FYI @joshmgross

MikeMcC399 commented 1 month ago

@joshmgross

Just to be sure, are you (or somebody else) going to release a new version of @actions/cache now?

Edit: removed misleading screenshot of @actions/cache@3.2.4

joshmgross commented 1 month ago

@MikeMcC399 yes I'm planning to upgrade the packages that depend on @actions/core.

It's worth noting that @actions/cache will still depend on uuid through @azure/core-http though:

❯ npm why uuid
uuid@8.3.2
node_modules/@azure/core-http/node_modules/uuid
  uuid@"^8.3.0" from @azure/core-http@3.0.2
  node_modules/@azure/core-http
    @azure/core-http@"^3.0.0" from @azure/storage-blob@12.15.0
    node_modules/@azure/storage-blob
      @azure/storage-blob@"^12.13.0" from the root project

uuid@8.3.2
node_modules/@azure/ms-rest-js/node_modules/uuid
  uuid@"^8.3.2" from @azure/ms-rest-js@2.7.0
  node_modules/@azure/ms-rest-js
    @azure/ms-rest-js@"^2.6.0" from the root project
MikeMcC399 commented 1 month ago

@joshmgross

yes I'm planning to upgrade the packages that depend on @actions/core.

It's worth noting that @actions/cache will still depend on uuid through @azure/core-http though:

Thanks for the confirmation! Looking forward to new releases which no longer depend on a deprecated version of uuid.