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.

Sign up for GitHub

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

Jump to bottom

[fix] node v10.0 lacks fs.promises #2654

Merged
merged 1 commit into from Feb 12, 2021
+24 −4
Merged

[fix] node v10.0 lacks fs.promises #2654

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/ci.yml
View file
Open in desktop
Expand Up @@ -41,7 +41,7 @@ jobs:
strategy:
fail-fast: false
matrix:
node-version: [10.x, 12.x, 14.x]
node-version: ['10.1', 10.x, '12.1', 12.x, '14.1', 14.x]
platform:
- os: ubuntu-latest
shell: bash
Expand Down
4 changes: 3 additions & 1 deletion lib/shrinkwrap.js
View file
Open in desktop
@@ -1,5 +1,7 @@
const { resolve, basename } = require('path')
const { promises: { unlink } } = require('fs')
const util = require('util')
const fs = require('fs')
const { unlink } = fs.promises || util.promisify(fs.unlink)
Copy link

Choose a reason for hiding this comment

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

How does one destructure an unlink property from a promisified function value? It's not following the pattern of lib/utils/reify-finish.js (or any number of dependency updates to address missing fs.promises), so maybe the tests are secretly failing for shrinkwrap? "Not throwing" isn't the same as "correct behavior in alternate circumstance".

-const { unlink } = fs.promises || util.promisify(fs.unlink)
+const { unlink } = fs.promises || { unlink: util.promisify(fs.unlink) }

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

whoops, you're right. this was a mistake. i'll put up a quick PR to fix it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note that tests aren't actually running in node 10.0, so the failures there wouldn't have shown up in CI.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

#2688

const Arborist = require('@npmcli/arborist')
const log = require('npmlog')

Expand Down
4 changes: 3 additions & 1 deletion lib/utils/reify-finish.js
View file
Open in desktop
@@ -1,7 +1,9 @@
const reifyOutput = require('./reify-output.js')
const npm = require('../npm.js')
const ini = require('ini')
const {writeFile} = require('fs').promises
const util = require('util')
const fs = require('fs')
const { writeFile } = fs.promises || { writeFile: util.promisify(fs.writeFile) }
const {resolve} = require('path')

const reifyFinish = async arb => {
Expand Down
8 changes: 8 additions & 0 deletions test/lib/shrinkwrap.js
View file
Open in desktop
@@ -1,4 +1,5 @@
const t = require('tap')
const fs = require('fs')
const requireInject = require('require-inject')

const npm = {
Expand Down Expand Up @@ -327,3 +328,10 @@ t.test('shrinkwrap --global', t => {
t.end()
})
})

t.test('works without fs.promises', async t => {
t.doesNotThrow(() => requireInject('../../lib/shrinkwrap.js', {
...mocks,
fs: { ...fs, promises: null },
}))
})
10 changes: 9 additions & 1 deletion test/lib/utils/reify-finish.js
View file
Open in desktop
Expand Up @@ -20,7 +20,7 @@ let expectWrite = false
const realFs = require('fs')
const fs = {
...realFs,
promises: {
promises: realFs.promises && {
...realFs.promises,
writeFile: async (path, data) => {
if (!expectWrite)
Expand Down Expand Up @@ -78,3 +78,11 @@ t.test('should write if everything above passes', async t => {
const data = fs.readFileSync(`${path}/npmrc`, 'utf8').replace(/\r\n/g, '\n')
t.matchSnapshot(data, 'written config')
})

t.test('works without fs.promises', async t => {
t.doesNotThrow(() => requireInject('../../../lib/utils/reify-finish.js', {
fs: { ...fs, promises: null },
'../../../lib/npm.js': npm,
'../../../lib/utils/reify-output.js': reifyOutput,
}))
})