Fix: do not fail and exit properly on errors #4
Conversation
index.js
Outdated
| @@ -5,46 +5,69 @@ const ora = require('ora'); | |||
| const chalk = require('chalk'); | |||
| const prettyMs = require('pretty-ms'); | |||
|
|
|||
| const getMetaTag = (html, property) => { | |||
| const getMetaTag = (html, property, spinner) => { | |||
There was a problem hiding this comment.
Same as with getMetaTagContent.
index.js
Outdated
| }; | ||
|
|
||
| const getMetaTagContent = metaTagHtml => { | ||
| const getMetaTagContent = (metaTagHtml, property, spinner) => { |
There was a problem hiding this comment.
We shouldn't be passing property/spinner through here and manipulating the spinner inside the funtion. Instead the function should throw/return false and then handle that result elsewhere in the code.
| }; | ||
|
|
||
| module.exports = bundler => { | ||
| bundler.on('buildEnd', async () => { | ||
| if (process.env.NODE_ENV !== 'production') { | ||
| return; | ||
| } | ||
| console.log(''); |
There was a problem hiding this comment.
Whoops, good catch
|
I wrapped now the code in a |
|
@lukechilds ping |
There was a problem hiding this comment.
@JPeer264 sorry for the delay.
I wrapped now the code in a try/catch - This handles just one error at a time though. You think that is good like that?
Yes that's perfect. If an error is encountered we should fail early. It's not normally a good idea to keep on working on something if something's gone wrong.
index.js
Outdated
| @@ -7,14 +7,24 @@ const prettyMs = require('pretty-ms'); | |||
|
|
|||
| const getMetaTag = (html, property) => { | |||
| const regex = new RegExp(`<meta[^>]*property=["|']${property}["|'][^>]*>`, 'i'); | |||
| const executedRegex = regex.exec(html); | |||
There was a problem hiding this comment.
I think results is a better name for this variable.
index.js
Outdated
| }; | ||
|
|
||
| const getMetaTagContent = metaTagHtml => { | ||
| const getMetaTagContent = (metaTagHtml, property) => { |
There was a problem hiding this comment.
Still no need to be passing property in here.
We can grab it from the metaTagHtml. Or even just dump the entire contents of metaTagHtml to the console. It's probably not a lot of text.
|
|
||
| const end = Date.now(); | ||
| const symbol = hasErrors ? chalk.red('✖') : '✨ '; |
There was a problem hiding this comment.
Do we need a space in '✖' here to keep the style consistent with the '✨ ' symbol?
There was a problem hiding this comment.
In this case it looks better without a space, as spinner.fail does not have a space either:
index.js
Outdated
|
|
||
| const end = Date.now(); | ||
| const symbol = hasErrors ? chalk.red('✖') : '✨ '; | ||
| const text = hasErrors ? | ||
| chalk.red('Failed to fix og:image link. Please fix the above error.') : |
There was a problem hiding this comment.
Maybe remove the Please fix the above error. text here.
This could be triggered by fs.writeFileSync and it might not make sense to say fix whatever the output of that is.
Just Failed to fix og:image link. should be fine. If there is a clear error above it the user will know that's what needs attention.
|
@lukechilds I updated the PR. |
Co-authored-by: JPeer264 <[email protected]>
Closes #3