chore(NODE-6272): update dev dependencies and add dependabot config #24
Conversation
baileympearson
changed the title
aditi-khare-mongoDB
added
the
Primary Review
index.js
Outdated
| } | ||
| } | ||
|
|
||
| switch (platform) { | ||
| case 'android': |
There was a problem hiding this comment.
What does adding 'android' and 'ia32' cases do and what inspired you to make this change?
There was a problem hiding this comment.
This whole file is auto-generated by our tooling (see the comment at the top of the file). It seems like this file changed a bit when I upgraded the CLI, so I decided to regenerate it and check it in.
There was a problem hiding this comment.
It should be okay then, thanks for clarifying!
From my quick google search, seems like no semicolons aren't that unstandard in JS, but I'll leave this thread open for team review in case anyone else wants to chime in.
There was a problem hiding this comment.
Despite being auto generated I recall we did remove the unsupported platforms intentionally. I think we probably should've removed that comment, I think this file could be considered manually maintained now.
There was a problem hiding this comment.
Okay, so for now I have:
- added the napi-cli to the exclude list in the dependabot so we don't try to update automatically in the future
- I manually removed the two unsupported platforms and left this dependency upgrade
Let me know if that sounds good to everyone.
There was a problem hiding this comment.
I manually removed the two unsupported platforms
Which two? I'm seeing more that I don't recognize, freebsd, arm64 windows, riscv64 linux, etc. Shouldn't we be able to revert this 🤔 I am not sure what we're bringing in
While the changes seem generally correct and good I worry about changes like:
- return readFileSync('/usr/bin/ldd', 'utf8').includes('musl');
+ const lddPath = require('child_process').execSync('which ldd').toString().trim()
+ return readFileSync(lddPath, 'utf8').includes('musl');Turning a file read into a process launch and file read not being surfaced appropriately so folks aren't surprised by a compression library launching a subprocess.
There was a problem hiding this comment.
To clarify we should revert any changes to index.js, and we can update this in a follow-up (we can decide on priority in triage).
index.js
Outdated
| if (localFileExisted) { | ||
| nativeBinding = require('./zstd.win32-x64-msvc.node'); | ||
| nativeBinding = require('./zstd.win32-x64-msvc.node') | ||
| } else { | ||
| nativeBinding = require('@mongodb-js/zstd-win32-x64-msvc'); | ||
| nativeBinding = require('@mongodb-js/zstd-win32-x64-msvc') | ||
| } | ||
| } catch (e) { | ||
| loadError = e; | ||
| loadError = e |
There was a problem hiding this comment.
How come we are removing semicolons at the ends of a lot of these lines?
There was a problem hiding this comment.
(see the other thread)
index.js
Outdated
| } | ||
| } | ||
|
|
||
| switch (platform) { | ||
| case 'android': |
There was a problem hiding this comment.
It should be okay then, thanks for clarifying!
From my quick google search, seems like no semicolons aren't that unstandard in JS, but I'll leave this thread open for team review in case anyone else wants to chime in.
aditi-khare-mongoDB
added
Team Review
baileympearson
requested review from
nbbeeken
and removed request for
nbbeeken
Description
What is changing?
Updates development dependencies and adds a Rust/npm dependabot config.
Is there new documentation needed for these changes?
What is the motivation for this change?
Double check the following
npm run format:js && npm run format:rsscripttype(NODE-xxxx)[!]: descriptionfeat(NODE-1234)!: rewriting everything in coffeescript