Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: bump glob to 9.3.5 #304

Closed
wants to merge 1 commit into from
Closed

Conversation

theogravity
Copy link

@theogravity theogravity commented Mar 7, 2024

This updates the project to use glob@9.3.5 (not using glob@10 as that looks to be ESM-only and this project does not have ESM support).

The reason for updating glob is older versions of glob uses inflight, which has a medium-level vulun that triggers our Vanta SOC2 compliance monitoring for security issues.

This version of glob no longer works for node 14, so I bumped the engine requirements to 18.

@theogravity theogravity requested a review from a team as a code owner March 7, 2024 20:01
@@ -18,8 +16,13 @@ async function determineFileType (filename) {

module.exports = async function (dir, options) {
const metadata = {}
const crawled = await glob(dir, options)
let crawled = await glob(dir, options || {})
Copy link
Author

@theogravity theogravity Mar 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did try async function (dir, options = {}) but tests would fail with that.

wonder if using ?? {} would be better.

Copy link
Member

@dsanders11 dsanders11 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be marked as a breaking change (add BREAKING CHANGE: ... in the body of the commit message) so that it would trigger a v4 release on merge.

If we're going to do a breaking change we probably want to just jump to Node v18 rather than v16 which is already EOL.

But I'm not sure if we consider this worth introducing a breaking change to address.

@dsanders11
Copy link
Member

Tests appear to be failing on Windows.

@theogravity
Copy link
Author

@dsanders11 Addressed the comments.

Copy link
Member

@erickzhao erickzhao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if this is a complete solution for the purposes of resolving security scanner warnings because we still have dependencies to glob@7.2.0, glob@^7.0.5, glob@^7.1.3 as per the yarn.lock file via various transient dependencies.

@theogravity
Copy link
Author

theogravity commented Mar 7, 2024

I'm not sure if this is a complete solution for the purposes of resolving security scanner warnings because we still have dependencies to glob@7.2.0, glob@^7.0.5, glob@^7.1.3 as per the yarn.lock file via various transient dependencies.

The transients are only on devDeps though. At least for our scanner, it only cares about deps for node_module installs.

PS C:\Users\theo\projects\asar> npm ls glob
@electron/asar@0.0.0-development C:\Users\theo\projects\asar
+-- electron-mocha@11.0.2
| `-- mocha@9.2.2
|   `-- glob@7.2.0
+-- glob@9.3.5
+-- mocha@10.1.0
| `-- glob@7.2.0
+-- rimraf@3.0.2
| `-- glob@7.2.3
`-- standard@14.3.4
  +-- eslint@6.8.0
  | `-- file-entry-cache@5.0.1
  |   `-- flat-cache@2.0.1
  |     `-- rimraf@2.6.3
  |       `-- glob@7.2.3
  `-- standard-engine@12.1.0
    `-- deglob@4.0.1
      `-- glob@7.2.3

BREAKING CHANGE: require Node 18.18.0
Copy link
Member

@ckerr ckerr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code changes LGTM, approving on that part.

@dsanders11 is right about the node bump / breaking change question. I don't have a strong opinion there except that we probably shouldn't depend on EOL packages

@erickzhao is right that there are still other devdep vectors do depend on old versions of glob, but fixing one is better than fixing zero 🤷‍♂️

@hallelk
Copy link

hallelk commented Jul 21, 2024

bump

@theogravity theogravity closed this by deleting the head repository Aug 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants