Skip to content

Fix crash on first launch (#78, #90) - #93

Merged
Jocs merged 1 commit into
marktext:masterfrom
fxha:FixStartupCrash
Mar 29, 2018
Merged

Jocs merged 1 commit into
marktext:masterfrom
fxha:FixStartupCrash

Conversation

@fxha

@fxha fxha commented Mar 28, 2018

Copy link
Copy Markdown
Contributor
Q A
Bug fix? yes
Fixed tickets #78 and #90
License MIT

Comment thread src/main/preference.js
this.userDataPath = userDataPath

if (!fs.existsSync(userDataPath)) {
mkdir(getPath('userData'))

@Jocs Jocs Mar 29, 2018 •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I suppose this will cause error too when getPath('appData') is also not existed?

I am newer in nodejs, I think a ensureDir method needed after I do some research.

const { spawnSync } = require('child_process')

const ensureDir = dir => {
  spawnSync('mkdir', ['-p', dir])
}
// or
const fse = require('node-fs-extra')
const ensureDir = dir => {
  fse.ensureDirSync(dir)
}

What's your opinion?

@fxha fxha Mar 29, 2018 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I suppose this will cause error too when getPath('appData') is also not existed?

Weird... Seems to be a electron fail. Elsewise fs.mkdirSync(...) will create all directories.

What's your opinion?

No this won't work on Windows and fs.mkdirSync(...) uses mkdir on Unix.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ok

@fxha fxha Mar 29, 2018 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Weird... Seems to be a electron fail.

This means that if you delete your appData directory electron will throw an exception. Which is weird. Maybe we should check this or use a fallback directory, but actually the directory should always exist.

node does not create all directories, so we have to use your suggestion.

const fse = require('node-fs-extra')
const ensureDir = dir => {
  fse.ensureDirSync(dir)
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I also prefer the second method, 👍 go ahead!

Comment thread src/main/utils.js
fs.mkdirSync(dirPath)
} catch (e) {
if (e.code !== 'EEXIST') {
throw e

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

use log method to write error log to log file

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think following code is the best case, otherwise Mark Text will crash because of no existing directory.

log(e)
throw e

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ok

@Jocs

Jocs commented Mar 29, 2018

Copy link
Copy Markdown
Member

@fxha great!

@Jocs

Jocs commented Mar 29, 2018

Copy link
Copy Markdown
Member

@fxha aha, please update the CHANGE_LOG.md after every bug fix、 add new featues or optimization. and mention related PR and issues.

Thank you.

@fxha
fxha force-pushed the FixStartupCrash branch from bb6e030 to f4bb964 Compare March 29, 2018 09:28
@Jocs
Jocs merged commit 5826367 into marktext:master Mar 29, 2018
@fxha
fxha deleted the FixStartupCrash branch March 29, 2018 10:48
thimbleberrysystems pushed a commit to thimbleberrysystems/WordBird that referenced this pull request Jun 21, 2026
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