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(node): Electron apps crashing #367

Merged
merged 2 commits into from
Mar 22, 2023

Conversation

Araxeus
Copy link
Contributor

@Araxeus Araxeus commented Mar 22, 2023

fix(node) Electron apps crashing

Description

Inside a app.asar file, the package.json might get trimmed and the bugs_url might be missing

repo_url conditional check was added for good measure

Fixes #365

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • I have checked my code and corrected any misspellings

BEGIN_COMMIT_OVERRIDE
fix(node): Electron apps crashing (#367)
END_COMMIT_OVERRIDE

Inside a `app.asar` file, the package.json might get trimmed and the `bugs_url` might be missing

`repo_url` conditional check was added for good measure
@Araxeus
Copy link
Contributor Author

Araxeus commented Mar 22, 2023

hotfix would be nice :)

(btw there is a commit message, so might be better not to squash)

Copy link
Owner

@LuanRT LuanRT left a comment

Choose a reason for hiding this comment

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

Did some tests and it's safe to assume homepage is always there from what I see. So to avoid hard coding stuff I suggest something like this:

//...
const package_json = JSON.parse(readFileSync(path.resolve(__dirname__, is_cjs ? '../package.json' : '../../package.json'), 'utf-8'));
const repo_url = package_json.homepage.split('#')[0];
//...
Platform.load({
  runtime: 'node',
  info: {
    version: package_json.version,
    bugs_url: package_json.bugs?.url || `${repo_url}/issues`,
    repo_url
   }
  //...
});

@Araxeus Araxeus requested a review from LuanRT March 22, 2023 20:04
@Araxeus
Copy link
Contributor Author

Araxeus commented Mar 22, 2023

Done, I did also add a ? to package_json.homepage just to avoid some obscure future conflict.

Copy link
Owner

@LuanRT LuanRT left a comment

Choose a reason for hiding this comment

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

Awesome. Looks good.
Also, there will be a release today but first I'll review #366.
Thank you for reporting & helping fix this issue!

@LuanRT LuanRT merged commit e7eacd9 into LuanRT:main Mar 22, 2023
@absidue
Copy link
Collaborator

absidue commented Mar 22, 2023

@LuanRT can you please add this to the bottom of this pull request's body, so that release please adds it to the changelog (the title and commit message are missing a :, so release please didn't detect it):

BEGIN_COMMIT_OVERRIDE
fix(node): Electron apps crashing (#367)
END_COMMIT_OVERRIDE

see #351 for an example

@LuanRT LuanRT changed the title fix(node) Electron apps crashing fix(node): Electron apps crashing Mar 22, 2023
@absidue
Copy link
Collaborator

absidue commented Mar 22, 2023

thanks, release please will pick up the override next time it runs, so when the next/another pull request is merged

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.

4.0.1 Error thrown in built Electron app
3 participants