Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Fix chance of having a lost window #9912

Merged
merged 1 commit into from
Jul 14, 2017
Merged

Fix chance of having a lost window #9912

merged 1 commit into from
Jul 14, 2017

Conversation

bbondy
Copy link
Member

@bbondy bbondy commented Jul 8, 2017

Fix #9806

Submitter Checklist:

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Ran git rebase -i to squash commits (if needed).
  • Tagged reviewers and labelled the pull request as needed.

Test Plan:
npm run unittest -- --grep="sessionStoreShutdown unit tests"

npm run watch-test
npm run test -- --grep="sessionStore test"

Reviewer Checklist:

Tests

  • Adequate test coverage exists to prevent regressions
  • Tests should be independent and work correctly when run individually or as a suite ref
  • New files have MPL2 license header

@bbondy bbondy added this to the 0.17.14 (Release Channel) milestone Jul 8, 2017
@bbondy bbondy self-assigned this Jul 8, 2017
@bbondy bbondy modified the milestones: 0.18.x (Beta Channel), 0.17.14 (Release Channel) Jul 10, 2017
@bbondy bbondy force-pushed the 9806 branch 3 times, most recently from 38280c3 to 2ebeca9 Compare July 13, 2017 14:16
@bbondy
Copy link
Member Author

bbondy commented Jul 13, 2017

Ready for review pls

Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

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

++

// exit cleanly on signals
['SIGTERM', 'SIGHUP', 'SIGINT', 'SIGBREAK'].forEach((signal) => {
;['SIGTERM', 'SIGHUP', 'SIGINT', 'SIGBREAK'].forEach((signal) => {
Copy link
Member

Choose a reason for hiding this comment

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

Is this semi-colon intentional?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I think it is. Because we don't use ; and you have } in the last line and this line starts with [, so this could lead into errors

Copy link
Member Author

Choose a reason for hiding this comment

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

yep it's a good practice to start lines like that with a semicolon so you don't need to worry about moved code and the preceding line of code.

require('../../app/renderer/reducers/contextMenuReducer'),
// This should be included even in production builds since you can use
// an environment variable to show the Debug menu
require('../../app/renderer/reducers/debugReducer')
Copy link
Member

Choose a reason for hiding this comment

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

❤️


describe('sessionStore', function () {
describe('sessionStore test', function () {
Copy link
Member

Choose a reason for hiding this comment

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

These tests are failing for me every time ☹️ ...but it looks good in Travis-ci? 🤔

Copy link
Member

Choose a reason for hiding this comment

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

OK just ran them again (3rd or 4th attempt). Worked great 😄 👍

@bsclifton bsclifton merged commit 69dfb59 into master Jul 14, 2017
@bsclifton bsclifton deleted the 9806 branch July 14, 2017 08:18
bsclifton added a commit that referenced this pull request Jul 14, 2017
Fix chance of having a lost window
@bsclifton
Copy link
Member

bsclifton commented Jul 14, 2017

This cherry-picked into 0.19.x without issues... but I had trouble cherry-picking into 0.18.x 😦

@bbondy can you help me resolve the conflict?

bbondy pushed a commit that referenced this pull request Jul 14, 2017
Fix chance of having a lost window
@bbondy
Copy link
Member Author

bbondy commented Jul 14, 2017

merged to 0.18.x

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants