- Notifications
You must be signed in to change notification settings - Fork406
Update to promise api for some methods in the electron API#2626
Uh oh!
There was an error while loading.Please reload this page.
Conversation
codecovbot commentedFeb 13, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thanks for tackling this! I'll be happy to merge if we can resolve:
- Using
async/awaitinstead 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
showOpenDialoghas changed.
lib/views/directory-select.js Outdated
| properties:['openDirectory','createDirectory','promptToCreate'], | ||
| },filePaths=>{ | ||
| if(filePaths!==undefined){ | ||
| }).then(({filePaths})=>{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think I'd prefer to use theasync/await style to call the Promise-based APIs here for consistency, if you don't mind 🙇🏻 .
Also, it looks like the return value ofshowOpenDialog has changed, too; it now returns anObject with certain properties:
So I think this call would be something like:
chooseDirectory=async()=>{const{canceled, filePaths}=awaitthis.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 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Hah! I totally missed at first read that you'd already added the{filePaths} destructuring in your.then callback argument.
| describe('clicking the directory button',function(){ | ||
| it('populates the destination path buffer on accept',asyncfunction(){ | ||
| constshowOpenDialog=sinon.stub().callsArgWith(2,['/some/directory/path']); | ||
| constshowOpenDialog=sinon.stub().returns(Promise.resolve({filePaths:['/some/directory/path']})); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
👍 Perfect.
| `; | ||
| awaitnewPromise(resolve=>browserWindow.webContents.executeJavaScript(script,resolve)); | ||
| awaitbrowserWindow.webContents.executeJavaScript(script); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
🙇🏻
i ll get to requested changes soon |
@smashwilson if atom 1.55 is based on electron 6 those api change will work. |
Looks like Atom 1.55 uses Electron 6.1.12: https://github.com/atom/atom/blob/24274440ec3e9c296a20daa3ad72aa57c074024a/package.json#L15 And here are the So you're correct, looks like we're good 👍🏻 |
| if(filePaths.length){ | ||
| this.props.buffer.setText(filePaths[0]); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
👍 Perfect!
lib/views/git-timings-view.js Outdated
| buffer.saveAs(filename); | ||
| }).then(({filePath})=>{ | ||
| if(!filePath){return;} | ||
| buffer.saveAs(filePath); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Can we use async/await here too?
lib/views/git-timings-view.js Outdated
| },asyncfilenames=>{ | ||
| if(!filenames){return;} | ||
| constfilename=filenames[0]; | ||
| }).then(async({filePaths})=>{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
And here ☝🏻
Sorry, should have clarified that I meant at all three callsites on the first review 👀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
oh ok no issue, i can convert them, i m still here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Excellent, thank you. Will merge when builds are green 🍏
Is it possible to get a new version published? what is your procedure around that? |
Ah, to make it easier to move forward the Electron upgrade work? Sure, I can do that: |
thanks@smashwilson appreciated |
Please be sure to read thecontributor's guide to the GitHub package before submitting any pull requests.
Requirements
Description of the Change
This PR aims at using the promise version of
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