-
-
Notifications
You must be signed in to change notification settings - Fork 26.9k
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
format UglifyJs error #2650
format UglifyJs error #2650
Conversation
😮 |
@@ -141,3 +142,30 @@ function copyPublicFolder() { | |||
filter: file => file !== paths.appHtml, | |||
}); | |||
} | |||
|
|||
function formatError(err) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's move this to react-dev-utils
. Something like formatBuildError
.
console.log( | ||
'UglifyJs could not parse the code from \n\n', | ||
chalk.yellow( | ||
err.stack.split('\n')[1].split('[')[1].split('][')[0].replace(']', '') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer if we checked against a regex (and extracted from it) instead.
So that if it doesn't match, we don't attempt to parse.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my regex is not very good, the string from err.stack.split('\n')[1]
looks like this
Unexpected token: name (A) [./packages/react-scripts/~/asdf/index.js:1,0][static/js/main.6078c716.js:9605,6]
Haven't developed the muscle memory for regexing that 😢
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use https://regex101.com/ for testing regexes.
I don't mind split/joining too much either, but then we need to test that it matches the format first somehow. Before we attempt to parse. So that we don't end up with garbage.
if (typeof message === 'string' && message.indexOf('from UglifyJs') !== -1) { | ||
try { | ||
console.log( | ||
'UglifyJs could not parse the code from \n\n', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's say Failed to minify code from
'\n' | ||
); | ||
} catch (e) { | ||
console.log('UglifyJs could not process the code.', err); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's say Failed to minify the code.
console.log('UglifyJs could not process the code.', err); | ||
} | ||
console.log( | ||
'Please check your dependencies for any untranspiled es6 code and raise an issue with \n' + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll want to tweak this message but let's fix other issues first.
packages/react-dev-utils/README.md
Outdated
@@ -187,7 +187,7 @@ measureFileSizesBeforeBuild(buildFolder).then(previousFileSizes => { | |||
}); | |||
``` | |||
|
|||
#### `formatWebpackMessages({errors: Array<string>, warnings: Array<string>}): {errors: Array<string>, warnings: Array<string>}` | |||
#### `formatwebpackmessages({errors: Array<string>, warnings: Array<string>}): {errors: Array<string>, warnings: Array<string>}` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whoops
const get = require('lodash/get'); | ||
const chalk = require('chalk'); | ||
|
||
module.exports = function formatBuildError(err) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm really not fond of this name since it does more than just formatting the error message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any suggestion? haven't got anything better pops up...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about printBuildError
?
console.log( | ||
'Failed to minify the code from \n\n', | ||
chalk.yellow( | ||
/Unexpected token:(.+)\[(.+)\]\[(.+)\]/.exec(err.stack)[2] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could not match or be undefined (in the future if the text format changes); we probably shouldn't assume it's always going to be OK (we don't want this method throwing an error).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I don't think I really like how the line / col is shown -- can we make this more human readable?
./bla/bla.js, line 52:1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (typeof message === 'string' && message.indexOf('from UglifyJs') !== -1) { | ||
try { | ||
console.log( | ||
'Failed to minify the code from \n\n', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about a colon?
ready for more review! |
packages/react-dev-utils/README.md
Outdated
const formatBuildError = require('react-dev-utils/formatBuildError') | ||
try { | ||
build() | ||
} catch(e){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: add missing space
const get = require('lodash/get'); | ||
const chalk = require('chalk'); | ||
|
||
module.exports = function formatBuildError(err) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about printBuildError
?
'Please check your dependencies for any untranspiled es6 code and raise an issue with \n' + | ||
'the author. \n' + | ||
'\nIf you need to use the module right now, you can try placing the source in ./src \n' + | ||
'and we will transpile it for you.' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a huge fan of this message. Can we add an entry for this into the User Guide and link to it instead?
packages/react-dev-utils/README.md
Outdated
@@ -220,6 +220,20 @@ compiler.plugin('done', function(stats) { | |||
}); | |||
``` | |||
|
|||
#### `printBuildError(error: Object): String` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume this should return void
.
packages/react-dev-utils/README.md
Outdated
#### `printBuildError(error: Object): String` | ||
|
||
Prettify some known build errors. | ||
Pass an Error object to log a prettified error message in the console |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: missing period at end
@@ -1998,6 +1999,16 @@ moment.locale('fr'); | |||
|
|||
This will only work for locales that have been explicitly imported before. | |||
|
|||
### `npm run build` fails to minify | |||
|
|||
Some dependencies may be shipping their source which our minify can't process. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's simplify this for now:
You may occasionally find a package you depend on needs compiled or ships code for a non-browser environment.<br>
This is considered poor practice in the ecosystem and does not have an escape hatch in Create React App.<br>
<br>
To resolve this:
1. Open an issue on the dependency's issue tracker and ask that the package be published pre-compiled (retaining ES6 Modules).
2. Fork the package and publish a corrected version yourself.
3. If the dependency is small enough, copy it to your `src/` folder and treat it as application code.
} | ||
console.log('Read more here:'); | ||
console.log( | ||
'https://github.com/facebookincubator/create-react-app/blob/master/packages/react-scripts/template/README.md#npm-run-build-fails-to-minify\n' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's shorten this: http://bit.ly/2tRViJ9
LGTM, I trust you that this works. |
Hi there! This change is out in |
* format UglifyJs error * move formatBuildError to react-dev-utils * fix readme * use regex for plucking the path from stack * make path human readable and fallback to show error if regex not matched * rename to printBuildError and add link to the docs * fix link indentation * improve readibility + shorten link
did you forget to add lodash in the package.json? |
Ah, it appears so! Interesting that the CI passed; is this causing you an issue? Can you submit a PR adding the dependency please? |
* commit 'bfaee410c502a95076a6bd89721c76ca08e15f7b': (39 commits) Publish Prepare for 1.0.11 release (facebook#2924) Update dev deps (facebook#2923) Update README.md Use env variable to disable source maps (facebook#2818) Make formatWebpackMessages return all messages (facebook#2834) Adjust the `checkIfOnline` check if in a corporate proxy environment (facebook#2884) Fix the order of arguments in spawned child proc (facebook#2913) Feature/webpack 3 4 (facebook#2875) Allow importing package.json (facebook#2468) Re-enable flowtype warning (facebook#2718) Format UglifyJs error (facebook#2650) Unstage yarn.lock pre-commit (facebook#2700) Update README.md Update README.md Add Electrode to alternatives (facebook#2728) Fix parsing HTML/JSX tags to real elements (facebook#2796) Update webpack version note (facebook#2798) Use modern syntax feature (facebook#2873) Allow use of scoped packages with a pinned version (facebook#2853) ... # Conflicts: # packages/react-scripts/config/webpack.config.dev.js # packages/react-scripts/config/webpack.config.prod.js # packages/react-scripts/package.json
* format UglifyJs error * move formatBuildError to react-dev-utils * fix readme * use regex for plucking the path from stack * make path human readable and fallback to show error if regex not matched * rename to printBuildError and add link to the docs * fix link indentation * improve readibility + shorten link
* format UglifyJs error * move formatBuildError to react-dev-utils * fix readme * use regex for plucking the path from stack * make path human readable and fallback to show error if regex not matched * rename to printBuildError and add link to the docs * fix link indentation * improve readibility + shorten link
* format UglifyJs error * move formatBuildError to react-dev-utils * fix readme * use regex for plucking the path from stack * make path human readable and fallback to show error if regex not matched * rename to printBuildError and add link to the docs * fix link indentation * improve readibility + shorten link
Fixing #2641