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

feat: check application package #355

Merged

Conversation

demetris-manikas
Copy link
Contributor

No description provided.

@mention-bot
Copy link

By analyzing the blame information on this pull request, we identified @develar and @pauliusuza to be potential reviewers

@@ -24,6 +24,8 @@ export interface AppMetadata extends Metadata {
*/
readonly description: string

readonly main?: string
Copy link
Member

Choose a reason for hiding this comment

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

I think we should not add such option. User should use standard package.json main. And, so, our check should check such field in the package.json.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I understood correctly interface AppMetadata holds all "useful" properties of the application package.json for the sake ot typesafety. I thought I only gave access to main (an already existent option). Please correct me if I am wrong.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, you are right.

@develar
Copy link
Member

develar commented Apr 25, 2016

I think, we should perform this check before pack to asar due to performance reason. i.e. — when app dir detected, we check it. Before pack, not after.

@demetris-manikas
Copy link
Contributor Author

demetris-manikas commented Apr 25, 2016

Well, then your comment that I should be aware of asar what was it for?
That is what made me do the check after the packaging.

@demetris-manikas
Copy link
Contributor Author

So should I apply the validation before or after packaging?

@develar
Copy link
Member

develar commented Apr 25, 2016

Well, then your comment that I should be aware of asar what was it for?

Sorry, it was just a proposal. After your changes I see that we can just use app dir.

Also, it will fix OS X — currently, it is broken because on OS X path to app.asar is different — `Contents/Resources/app.asar

On other hand — I doubt that it will be a bottleneck (it seems, getting contents of asar file is very quickly). And, strictly speaking, it is better to check the final result, not intermediate.

So, 👍 to current ASAR check, just fix OS X.

}

private async statPackageFile(appOutDir: string, packageFile: string, isAsar: boolean): Promise<any> {
let fpath = path.resolve("/", packageFile)
Copy link
Member

@develar develar Apr 25, 2016

Choose a reason for hiding this comment

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

Please consider to use const here (and in other places).

@develar
Copy link
Member

develar commented Apr 25, 2016

Also, please add asar dependency explicitly (currently, we inherit it from electron-packager).

@demetris-manikas
Copy link
Contributor Author

I believe all is done.

@develar develar merged commit 27faf73 into electron-userland:master Apr 25, 2016
@develar
Copy link
Member

develar commented Apr 25, 2016

Thank you!

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.

3 participants