Update to promise api for some methods in the electron API #2626

asturur · 2021-02-13T18:02:31Z

Please be sure to read the contributor's guide to the GitHub package before submitting any pull requests.

Requirements

Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.

Description of the Change

This PR aims at using the promise version of

showSaveDialog,
showOpenDialog,
executeJavascript

electron API

In order to ease the upgrade path to newer versions of electron where the callback is no more supported.

Screenshot or Gif

Applicable Issues

codecov · 2021-02-13T18:10:15Z

Codecov Report

Merging #2626 (44d5d86) into master (8c07644) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2626      +/-   ##
==========================================
- Coverage   93.46%   93.46%   -0.01%     
==========================================
  Files         237      237              
  Lines       13215    13213       -2     
  Branches     1900     1900              
==========================================
- Hits        12352    12349       -3     
- Misses        863      864       +1

Impacted Files	Coverage Δ
lib/views/directory-select.js	`100.00% <100.00%> (ø)`
lib/atom/gutter.js	`90.47% <0.00%> (-2.39%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8c07644...44d5d86. Read the comment docs.

smashwilson

Thanks for tackling this! I'll be happy to merge if we can resolve:

Using async/await instead of .then (which is just a style/consistency thing), and
Confirming that this will still work against Atom 1.55, or finding a way to bridge the Electron API gap, since the return value of showOpenDialog has changed.

smashwilson · 2021-02-14T15:16:41Z

lib/views/directory-select.js

    this.props.showOpenDialog(this.props.currentWindow, {
      defaultPath: this.props.buffer.getText(),
      properties: ['openDirectory', 'createDirectory', 'promptToCreate'],
-    }, filePaths => {
-      if (filePaths !== undefined) {
+    }).then(({filePaths}) => {


I think I'd prefer to use the async/await style to call the Promise-based APIs here for consistency, if you don't mind 🙇🏻 .

Also, it looks like the return value of showOpenDialog has changed, too; it now returns an Object with certain properties:

https://github.com/electron/electron/blob/v9.4.3/docs/api/dialog.md#dialogshowopendialogbrowserwindow-options

So I think this call would be something like:

chooseDirectory = async () => { const {canceled, filePaths} = await this.props.showOpenDialog(this.props.currentWindow, { defaultPath: this.props.buffer.getText(), properties: ['openDirectory', 'createDirectory', 'promptToCreate'], }; if (!canceled && filePaths.length > 0) { this.props.buffer.setText(filePaths[0]); } }

But, we have to make sure that the different return value is compatible with the call signature on the Electron versions back to Atom 1.55 (the current beta), too, so we don't break our ability to backport 🤔

Hah! I totally missed at first read that you'd already added the {filePaths} destructuring in your .then callback argument.

smashwilson · 2021-02-14T15:17:59Z

test/views/directory-select.test.js

@@ -47,7 +47,7 @@ describe('DirectorySelect', function() {

  describe('clicking the directory button', function() {
    it('populates the destination path buffer on accept', async function() {
-      const showOpenDialog = sinon.stub().callsArgWith(2, ['/some/directory/path']);
+      const showOpenDialog = sinon.stub().returns(Promise.resolve({filePaths: ['/some/directory/path']}));


👍 Perfect.

smashwilson · 2021-02-14T15:18:08Z

test/worker-manager.test.js

@@ -167,7 +167,7 @@ describe('WorkerManager', function() {
      });
      `;

-      await new Promise(resolve => browserWindow.webContents.executeJavaScript(script, resolve));
+      await browserWindow.webContents.executeJavaScript(script);


asturur · 2021-02-14T15:49:18Z

i ll get to requested changes soon

asturur · 2021-02-14T17:00:17Z

@smashwilson if atom 1.55 is based on electron 6 those api change will work.
The callback was removed from documentation on electron 6 and still supported, but this promise API has been introduced in 6.0

smashwilson · 2021-02-14T17:05:21Z

Looks like Atom 1.55 uses Electron 6.1.12:

https://github.com/atom/atom/blob/24274440ec3e9c296a20daa3ad72aa57c074024a/package.json#L15

And here are the showOpenDialog docs for that version:

https://github.com/electron/electron/blob/v6.1.12/docs/api/dialog.md#dialogshowopendialogbrowserwindow-options

So you're correct, looks like we're good 👍🏻

smashwilson · 2021-02-14T17:05:54Z

lib/views/directory-select.js

+    if (filePaths.length) {
+      this.props.buffer.setText(filePaths[0]);
+    }
+  }


👍 Perfect!

smashwilson · 2021-02-14T17:06:06Z

lib/views/git-timings-view.js

-      buffer.saveAs(filename);
+    }).then(({filePath}) => {
+      if (!filePath) { return; }
+      buffer.saveAs(filePath);


Can we use async/await here too?

smashwilson · 2021-02-14T17:06:45Z

lib/views/git-timings-view.js

-    }, async filenames => {
-      if (!filenames) { return; }
-      const filename = filenames[0];
+    }).then(async ({filePaths}) => {


And here ☝🏻

Sorry, should have clarified that I meant at all three callsites on the first review 👀

oh ok no issue, i can convert them, i m still here.

smashwilson

Excellent, thank you. Will merge when builds are green 🍏

asturur · 2021-02-14T18:03:01Z

Is it possible to get a new version published? what is your procedure around that?

smashwilson · 2021-02-16T14:32:03Z

Ah, to make it easier to move forward the Electron upgrade work? Sure, I can do that:

atom-github ➤ apm publish patch
Preparing and tagging a new version ✓
Pushing v0.36.8 tag ✓
Publishing github@v0.36.8 ✓

asturur · 2021-02-16T15:29:02Z

thanks @smashwilson appreciated

changes to promise

01dcf62

This was referenced Feb 13, 2021

Update uses of Electron API calls that require Promises in Electron 7+ (callbacks are deprecated) #2624

Closed

Upgrade to electron 9.3.5 atom/atom#21777

Merged

asturur added 3 commits February 13, 2021 19:17

fix a test

1d9de01

ok now maybe fixed tests

bd6b8fb

damn hurry

50ddcb9

DeeDeeG mentioned this pull request Feb 13, 2021

dialog.showOpenDialog is a promise, not a callback #2605

Closed

1 task

smashwilson reviewed Feb 14, 2021

View reviewed changes

use aync await syntax

0463c16

asturur requested a review from smashwilson February 14, 2021 17:00

smashwilson reviewed Feb 14, 2021

View reviewed changes

more async await

44d5d86

smashwilson approved these changes Feb 14, 2021

View reviewed changes

smashwilson merged commit b4896c9 into atom:master Feb 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update to promise api for some methods in the electron API #2626

Update to promise api for some methods in the electron API #2626

asturur commented Feb 13, 2021

codecov bot commented Feb 13, 2021 •

edited

Loading

smashwilson left a comment

smashwilson Feb 14, 2021

smashwilson Feb 14, 2021

smashwilson Feb 14, 2021

smashwilson Feb 14, 2021

asturur commented Feb 14, 2021

asturur commented Feb 14, 2021

smashwilson commented Feb 14, 2021

smashwilson Feb 14, 2021

smashwilson Feb 14, 2021

smashwilson Feb 14, 2021

asturur Feb 14, 2021

smashwilson left a comment

asturur commented Feb 14, 2021

smashwilson commented Feb 16, 2021

asturur commented Feb 16, 2021

Update to promise api for some methods in the electron API #2626

Update to promise api for some methods in the electron API #2626

Conversation

asturur commented Feb 13, 2021

Requirements

Description of the Change

Screenshot or Gif

Applicable Issues

codecov bot commented Feb 13, 2021 • edited Loading

Codecov Report

smashwilson 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

asturur commented Feb 14, 2021

asturur commented Feb 14, 2021

smashwilson commented Feb 14, 2021

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

smashwilson left a comment

Choose a reason for hiding this comment

asturur commented Feb 14, 2021

smashwilson commented Feb 16, 2021

asturur commented Feb 16, 2021

codecov bot commented Feb 13, 2021 •

edited

Loading