fix: some APIs modified for ASAR support cannot be util.promisify'ed #13845

miniak · 2018-07-29T12:33:23Z

Fixes #13458

Promisification of the following functions is being fixed:

fs.exists()
child_process.exec()
child_process.execFile()

Checklist

PR description included and stakeholders cc'd
npm test passes
tests are changed or added
PR title follows semantic commit guidelines

MarshallOfSound

.

MarshallOfSound · 2018-07-30T00:58:08Z

lib/common/asar.js

@@ -27,6 +28,12 @@
    return archive
  }

+  function assertNoCustomPromisify (func) {


I feel as though these assertions should live inside tests, not runtime code 👍

Something along the lines of

for (const key in originalFs) { if (originalFs[key][util.promisify.custom] && !fs[key][util.promisify.custom]) { // fail the test } }

@MarshallOfSound that's a really good point. I didn't notice the originalFs can be accessed in the tests. Done

MarshallOfSound · 2018-07-30T01:03:36Z

lib/common/asar.js

+      }
+      const archive = getOrCreateArchive(asarPath)
+      if (!archive) {
+        return Promise.resolve(false)


This is inconsistent with the fs.exists behavior above, in the case of an invalid archive it throws / calls back with an error. This method will fail transparently with a resolved false

@MarshallOfSound original fs.exists never throws, it always returns either true or false. I can make it reject in this case though.

MarshallOfSound · 2018-07-30T01:04:17Z

lib/common/asar.js

+
+        const archive = getOrCreateArchive(asarPath)
+        if (!archive) {
+          return new Promise(() => invalidArchiveError(asarPath))


This should be return Promise.reject(invalidArchiveError(asarPath))

@MarshallOfSound that won't work, invalidArchiveError throws the error

Oh I see, this will result in a rejected promise right? 😄

MarshallOfSound · 2018-07-30T01:05:10Z

lib/common/asar.js

+
+        const newPath = archive.copyFileOut(filePath)
+        if (!newPath) {
+          return new Promise(() => notFoundError(asarPath, filePath))


This should be return Promise.reject(notFoundError(asarPath, filePath))

@MarshallOfSound that won't work, notFoundError throws the error

MarshallOfSound · 2018-07-30T01:06:16Z

spec/asar-spec.js

@@ -644,6 +699,13 @@ describe('asar package', function () {
          done()
        })
      })
+
+      it('can be promisified', (done) => {
+        util.promisify(ChildProcess.exec)('echo ' + echo + ' foo bar').then(({ stdout }) => {


This is a promise so we don't need to use the done technique. We can just return the promise in the test and the test will finish when the promise is resolved or rejected.

@MarshallOfSound done

MarshallOfSound · 2018-07-30T01:06:48Z

spec/asar-spec.js

+        var p = path.join(fixtures, 'asar', 'a.asar', 'file1')
+        // eslint-disable-next-line
+        fs.exists(p, function (exists) {
+          assert.equal(exists, true)


Can we use expect from the chair package instead of assert please, it will help the work that @codebytere is doing to improve our test suite

@MarshallOfSound I don't want that to be back-ported to 2.0. It's small amount of code compared to the rest of the tests.

trop · 2018-08-01T03:08:12Z

An error occurred while attempting to backport this PR to "2-0-x", you will need to perform this backport manually

trop · 2018-08-01T03:09:35Z

We have automatically backported this PR to "3-0-x", please check out #13902

MarshallOfSound · 2018-08-01T03:26:40Z

@miniak

An error occurred while attempting to backport this PR to "2-0-x", you will need to perform this backport manually

☝️ if you want to get this into a 2.0.x release

…13845)

miniak requested a review from a team July 29, 2018 12:33

miniak mentioned this pull request Jul 29, 2018

Behaviour of util.promisify(childProcess.exec) inconsistent with node #13458

Closed

miniak force-pushed the miniak/fix-promisify branch 5 times, most recently from 53fd254 to ac480ad Compare July 29, 2018 13:17

miniak changed the title ~~[wip] fix: some APIs modified for ASAR support cannot be util.promisify'ed~~ fix: some APIs modified for ASAR support cannot be util.promisify'ed Jul 29, 2018

miniak requested review from zeke, zcbenz and MarshallOfSound July 29, 2018 13:18

miniak added target/1-8-x labels Jul 29, 2018

miniak force-pushed the miniak/fix-promisify branch from ac480ad to f5708dc Compare July 29, 2018 13:34

MarshallOfSound requested changes Jul 30, 2018

View reviewed changes

MarshallOfSound removed the target/1-8-x label Jul 30, 2018

miniak force-pushed the miniak/fix-promisify branch 2 times, most recently from 93191f4 to c0afb79 Compare July 30, 2018 08:46

fix: some APIs modified for ASAR support cannot be util.promisify'ed

c2cb29a

miniak force-pushed the miniak/fix-promisify branch from c0afb79 to c2cb29a Compare July 30, 2018 08:57

MarshallOfSound approved these changes Aug 1, 2018

View reviewed changes

MarshallOfSound merged commit c52b3d9 into master Aug 1, 2018

MarshallOfSound deleted the miniak/fix-promisify branch August 1, 2018 03:06

trop bot mentioned this pull request Aug 1, 2018

fix: some APIs modified for ASAR support cannot be util.promisify'ed (backport: 3-0-x) #13902

Merged

trop bot added merged/3-0-x and removed target/3-0-x labels Aug 1, 2018

miniak added a commit that referenced this pull request Aug 6, 2018

fix: some APIs modified for ASAR support cannot be util.promisify'ed (#…

724b47f

…13845)

miniak mentioned this pull request Aug 6, 2018

fix: some APIs modified for ASAR support cannot be util.promisify'ed (backport: 2-0-x) #13960

Merged

4 tasks

miniak added a commit that referenced this pull request Aug 6, 2018

fix: some APIs modified for ASAR support cannot be util.promisify'ed (#…

6a86831

…13845)

miniak added merged/2-0-x and removed target/2-0-x labels Aug 8, 2018

ckerr mentioned this pull request Dec 18, 2018

Draft 4.0.0 Release Notes #16117

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: some APIs modified for ASAR support cannot be util.promisify'ed #13845

fix: some APIs modified for ASAR support cannot be util.promisify'ed #13845

miniak commented Jul 29, 2018 •

edited

MarshallOfSound left a comment

MarshallOfSound Jul 30, 2018

MarshallOfSound Jul 30, 2018

miniak Jul 30, 2018

MarshallOfSound Jul 30, 2018

miniak Jul 30, 2018

MarshallOfSound Jul 30, 2018

miniak Jul 30, 2018

MarshallOfSound Jul 30, 2018

miniak Jul 30, 2018

MarshallOfSound Jul 30, 2018

miniak Jul 30, 2018

MarshallOfSound Jul 30, 2018

miniak Jul 30, 2018 •

edited

MarshallOfSound Jul 30, 2018

miniak Jul 30, 2018

trop bot commented Aug 1, 2018

trop bot commented Aug 1, 2018

MarshallOfSound commented Aug 1, 2018

fix: some APIs modified for ASAR support cannot be util.promisify'ed #13845

fix: some APIs modified for ASAR support cannot be util.promisify'ed #13845

Conversation

miniak commented Jul 29, 2018 • edited

Checklist

MarshallOfSound left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miniak Jul 30, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trop bot commented Aug 1, 2018

trop bot commented Aug 1, 2018

MarshallOfSound commented Aug 1, 2018

miniak commented Jul 29, 2018 •

edited

miniak Jul 30, 2018 •

edited