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

Automating entitlement app group only if there is entitlements #52

Merged
merged 1 commit into from
Jun 1, 2016

Conversation

develar
Copy link
Contributor

@develar develar commented Jun 1, 2016

Otherwise if platform darwin no entitlements by default and sign failed because preAutoEntitlementAppGroupAsync does readFileAsync(opts.entitlements, 'utf8')

}
}
return promise
Copy link
Contributor

@sethlu sethlu Jun 1, 2016

Choose a reason for hiding this comment

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

@develar I think we still need to pass down something like return Promise.resolve() otherwise the promise chain breaks.

Copy link
Contributor Author

@develar develar Jun 1, 2016

Choose a reason for hiding this comment

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

No,

If the value is a thenable (i.e. has a then method), the returned promise will "follow" that thenable, adopting its eventual state; otherwise the returned promise will be fulfilled with the value.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise

If you want to state that return is not missed, return null: return null. FYI: e.g. typescript doesn't require explicit return in the async function even if there are other return in the function. Just because it is very convenient.

Copy link
Contributor

@sethlu sethlu Jun 1, 2016

Choose a reason for hiding this comment

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

@develar I don't know but a similar case has caused a slight issue previously... not sure if being a same cause: 184985a. Would you mind having a check?

Probably that's a different case because of returning a promise obj so what following uses it before continuing .then()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it was an error in my PR (https://github.com/electron-userland/electron-osx-sign/pull/45/files#diff-168726dbe96b3ce427e7fedce31bb0bcR192) It is promise pitfall, when you forget to return promise result in the then hander :(

Copy link
Contributor

Choose a reason for hiding this comment

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

@develar No worries, thanks for clarifying 😸 I didn't notice it earlier as well.

@sethlu
Copy link
Contributor

sethlu commented Jun 1, 2016

@develar Thanks for bringing up these issue fixes and the updates on the typescript; 👍 really appreciated them! Do you mind having a look at some of the comments on file changes?

@sethlu sethlu merged commit 3e0535e into electron:master Jun 1, 2016
@sethlu
Copy link
Contributor

sethlu commented Jun 1, 2016

@develar PR merged; thanks for working on these improvements. 👍

@sethlu sethlu added this to the Release v0.4.0 milestone Jun 8, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants