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

Improve minifiers to capture error from UglifyJS. #8414

Merged
merged 9 commits into from Feb 28, 2017

Conversation

@abernix
Copy link
Member

@abernix abernix commented Feb 23, 2017

This is a stop-gap until #8378 is complete.

The error messages which come from UglifyJS tend to be quite cryptic, as
seen in issues like #8370 or #8020. The file,
line, and column are provided, however the message is garbled and the
stacktrace long and acutely harrowing. Since these errors are occurring
on automatically concatenated files, even the line number is sometimes
not helpful. Additionally, sourceMaps are not available in production
builds, intentionally. (I wasn't able to access them from
file.getSourceMap() or file.sourceMap at all.)

In addition to actually providing the name of the encapsulating file,
which provides _some_visibility, this commit implements a parser around
the UglifyJS error which detects the error and location information of
the error, seeks to the line in the concatenated source, reads the
inline filename, and provides it in the output.

Crude, but incredibly helpful in diagnosing this problem until a better
solution is reached.

@abernix abernix self-assigned this Feb 23, 2017
@abernix abernix requested a review from benjamn Feb 23, 2017
@abernix abernix changed the base branch from devel to master Feb 24, 2017
@abernix abernix changed the base branch from master to devel Feb 24, 2017
@abernix abernix force-pushed the abernix/feature/minifier-catch-error branch from 8fcbc80 to 5e5951d Feb 24, 2017
Copy link
Member

@hwillson hwillson left a comment

Hi @abernix - I've taken a look and everything looks great! I've outlined one small formatting suggestion. I ran into a bit of a problem using these minification changes when I tried them out, but I don't think the issue I encountered should hold these changes up. I'll outline the details in a separate comment. Thanks!

var parseErrorPath = contents[c - 2]
.substring(3)
.replace(/\s+\/\//, "")
;

This comment has been minimized.

@hwillson

hwillson Feb 24, 2017
Member

Maybe move this semicolon up a line? Looks a bit dangly here.

This comment has been minimized.

@abernix

abernix Feb 24, 2017
Author Member

Yeah, it's super dangly. In my personal coding-life, I enjoy leaving room to add new in-between methods and having a cleaner diff by turning it into a makeshift curly bracket (to show what it matches up with). That being said, this is a good call on your part and I've fixed it (as this style exists no-where in the code right now).

@hwillson
Copy link
Member

@hwillson hwillson commented Feb 24, 2017

Hi @abernix - Just one quick side note / gotcha I ran into with these changes. This is likely an edge case, and there is a workaround, but I'll outline the problem I ran into here (just in-case it comes up in the future).

I just tried these changes out against an app of mine that I was having minification problems with. This app uses a "packages for everything" type of structure, so something like:

/myapp
  package.json
  /packages
    /myapp-lib
    /myapp-core
    /myapp-utilities
    ...

This means the top level folder is really just a container, but I still run all meteor commands from within that top level folder. When Meteor builds from the top level folder, since it can't find a package.js (since the top level isn't a package) it assumes it's an app. This means that during the build process, one of the files that is passed into the minification process looks like this:

JsFile
  _source: 
   File {
     sourcePath: undefined,
     info: 'resource /app.js',
     sourceMap: null,
     sourceMapRoot: null,
     targetPath: 'app/app.js',
     url: '/app.js',
     cacheable: false,
     nodeModulesDirectories: {},
     assets: null,
     _contents: <Buffer >,
     _hashOfContents: null,
     _hash: null },
  _arch: undefined,
  _minifiedFiles: [] }

Since this file doesn’t really exist, it’s contents after calling UglifyJSMinify on it look like:

{ code: '', map: null }

In this specific case this is actually okay; the code value being an empty string is (sorta/kinda) valid, since the app.js file doesn't really exist. The old UglifyJSMinify code did this

allJs += UglifyJSMinify(file.getContentsAsString(), minifyOptions).code;

so the empty code value was just appended, and it kept going. The new changes don't allow this however

minified = UglifyJSMinify(file.getContentsAsString(), minifyOptions);
if (! minified.code) {
  throw new Error();
}

so the empty code value causes an error that looks like:

=> Errors prevented startup:                  
   
   While minifying app code:
   packages/minifyStdJS_plugin.js:147:17:  while minifying app/app.js
   at packages/minifyStdJS_plugin.js:147:17
   at Array.forEach (native)
   at UglifyJSMinifier.processFilesForBundle
   (packages/minifyStdJS_plugin.js:131:9)

The above being said, I don’t think this should impact your minification changes; the real issue is that the build process is passing an app.js file in for minification, when it doesn't exist. There is of course a workaround to all of this - by adding an empty package.js file in my top level project folder, the app.js file passed in then looks like:

{ code: 'var require=meteorInstall({"package.js":function(){}},{extensions:[".js",".json",".html",".css"]});require("./package.js");',
  map: null }

and let's the minification process continue.

Anyways, just a heads up in-case this comes up for anyone else (but now that we have ES2015 module support, the "packages for everything" approach is definitely less popular). Thanks!

@abernix
Copy link
Member Author

@abernix abernix commented Feb 24, 2017

@hwillson Great catch. Fixed with 9eb9ede if you want to try it again! I'm also curious if the error message you receive actually helps you find a problem! 😄

@abernix abernix force-pushed the abernix/feature/minifier-catch-error branch to 9eb9ede Feb 24, 2017
abernix added a commit that referenced this pull request Feb 24, 2017
In order to allow for a blank `app.js` which occurs in the case of
a Meteor app using a fully-"package"-based structure with no actual
application code in the top-level.  See #8414 for more.
@hwillson
Copy link
Member

@hwillson hwillson commented Feb 27, 2017

Hi @abernix - I just tested 9eb9ede - looks great! These changes are definitely helpful, so LGTM!

@abernix abernix added this to the Release 1.4.3.x milestone Feb 27, 2017
abernix added 6 commits Feb 21, 2017
The error messages which come from UglifyJS tend to be quite cryptic, as
seen in issues like #8370 or #8020.  The file,
line, and column are provided, however the message is garbled and the
stacktrace long and acutely harrowing.  Since these errors are occurring
on automatically concatenated files, even the line number is sometimes
not helpful.  Additionally, sourceMaps are not available in production
builds, intentionally.  (I wasn't able to access them from
`file.getSourceMap()` or `file.sourceMap` at all.)

In addition to actually providing the name of the encapsulating file,
which provides _some_visibility, this commit implements a parser around
the UglifyJS error which detects the error and location information of
the error, seeks to the line in the concatenated source, reads the
inline filename, and provides it in the output.

Crude, but incredibly helpful in diagnosing this problem until a better
solution is reached.
In order to allow for a blank `app.js` which occurs in the case of
a Meteor app using a fully-"package"-based structure with no actual
application code in the top-level.  See #8414 for more.
@abernix abernix force-pushed the abernix/feature/minifier-catch-error branch from 9eb9ede to c24cb71 Feb 28, 2017
@abernix abernix changed the base branch from devel to master Feb 28, 2017
abernix added 3 commits Feb 28, 2017
@abernix abernix merged commit 58a5fdb into master Feb 28, 2017
4 checks passed
4 checks passed
CLA Author has signed the Meteor CLA.
Details
ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@abernix
Copy link
Member Author

@abernix abernix commented Feb 28, 2017

This is now officially released as standard-minifier-js@1.2.3 and can be added by running the following on Meteor 1.4.x projects:

meteor update standard-minifier-js
abernix added a commit that referenced this pull request Feb 28, 2017
This was only intended to be there during the publishing of
`standard-minifier-js` as part of #8414 and is normally
not necessary as part of the `meteor publish-release` process.
@mwarren2
Copy link

@mwarren2 mwarren2 commented Mar 3, 2017

@abernix Thank you for this work.
I've been fighting with a weird uglify message for more than a week.
I installed 1.2.3 and got a laser-sharp message indicating offending code and line no

@abernix abernix deleted the abernix/feature/minifier-catch-error branch Mar 3, 2017
Copy link
Member

@benjamn benjamn left a comment

lgtm

@abernix abernix modified the milestones: Release 1.4.3.x, Release 1.4.3.2 Mar 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.