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.

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

fix(electron): Prevent promises both resolving and rejecting #1618

Merged

Conversation

27pchrisl
Copy link
Contributor

This change adds returns after all rejects, so that promises don't resolve after a rejection.

Copy link
Member

@jcesarmobile jcesarmobile left a comment

Choose a reason for hiding this comment

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

I've been reading and if you first reject a promise and then resolve it later, it won't have any effect. But also have read that it's recommended to return after reject to avoid extra code execution.

In this cases doesn't save much, but I'm going to merge anyway, thanks for the PR!

@jcesarmobile jcesarmobile changed the title Electron filesystem promises both resolve and reject fix(electron): filesystem promises both resolve and reject Jun 3, 2019
@jcesarmobile jcesarmobile changed the title fix(electron): filesystem promises both resolve and reject fix(electron): Prevent promises both resolving and rejecting Jun 3, 2019
@jcesarmobile jcesarmobile merged commit 43ea1dd into ionic-team:master Jun 3, 2019
@27pchrisl
Copy link
Contributor Author

Hi, I should have put this in the original ticket, but the specific way I ran into a problem was this code:

      this.NodeFS.stat(options.path, (err:any, stats:any) => {
        if(err)
          reject(err);    <== "err" exists, and so a rejection occured

        resolve({type: 'Not Available', size: stats.size, ctime: stats.ctimeMs, mtime: stats.mtimeMs, uri: lookupPath});    <== JS engine moves onto this line, where because of the err "stats" is undefined and so throws due to stats.size accessing a property of an undefined.
      });

@jcesarmobile
Copy link
Member

Ah, yeah, in that case it was a bug. Good catch!

27pchrisl pushed a commit to 27partners/capacitor that referenced this pull request Jun 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants