Uglifyjs: "First argument must be a string or Buffer"

[Martii]

Bug report or feature request?
BUG

ES5 or ES6+ input?
ES6+

Uglify version (uglifyjs -V)
3.0.3 and 3.0.4

JavaScript input
https://gist.github.com/Martii/1f8ddfa71c8a9246e138a97d1a90e3f6

The uglifyjs CLI command executed or minify() options used.
API via OpenUserJs/OpenUserJS.org/blob/3a02c4/controllers/scriptStorage.js#L676-L684

JavaScript output or error produced.

Message from 3.0.3

2017-05-12 10:58:57.197 +00:00: MINIFICATION WARNING (harmony):
  message: Data must be a string or a buffer
  installName: hadesnhat42/hadesnhat42(toan).user.js
  line: undefined col: undefined pos: undefined

NOTE line and col are undefined

3.0.4 trips the server with:

MINIFICATION WARNING (harmony):
  message: Data must be a string or a buffer
  installName: hadesnhat42/hadesnhat42(toan).user.js
  line: undefined col: undefined pos: undefined
_http_outgoing.js:458
    throw new TypeError('First argument must be a string or Buffer');
    ^

TypeError: First argument must be a string or Buffer
    at ServerResponse.OutgoingMessage.write (_http_outgoing.js:458:11)
    at ServerResponse.write (~/OpenUserJS.org1/node_modules/compression/index.js:88:18)
    at ServerResponse.res.write (~/OpenUserJS.org1/node_modules/express-minify/index.js:92:22)
    at PassThrough.<anonymous> (~/OpenUserJS.org1/controllers/scriptStorage.js:743:14)
    at emitNone (events.js:91:20)
    at PassThrough.emit (events.js:185:7)
    at endReadableNT (_stream_readable.js:974:12)
    at _combinedTickCallback (internal/process/next_tick.js:80:11)
    at process._tickDomainCallback (internal/process/next_tick.js:128:9)

NOTES
Some scripts minify but the gist definitely crashes server.

[Martii]

Stand corrected... 3.0.3 trips server as well.

[Martii]

Confirmed working (as expected) in last fork of 2.x (currently frozen at that commit) e.g. 3.x uglifyjs-es implementation is the culprit.

---

Any ideas?

[Martii]

@NGPixel

So one of the migration issues is that v2.x returns the untouched source string on UglifyJS errors in code and v3.x currently returns undefined in code... modified OUJS accordingly to prevent a server trip.

EDIT We have already received this message from our handler of:

... (clipped as our boog at this part of a post followup)

... so there is still some issue with 3.0.4... will try harmony branch here on that Userscript to see if that is resolved with our local pro.

---

Script with our error handling returns in the transaction headers:

...
Warning: 199 openuserjs.org MINIFICATION WARNING (harmony):   Unexpected token: punc (})   line: 269 col: 0 pos: 8409
...

So at least a DOC bug here or a MIGRATION/REGRESSION bug here of not returning the untouched data.

[alexlamsl]

Please provide a test case when only involves uglify-js/uglify-es.

[Martii]

Please provide a test case when only involves uglify-js/uglify-es.

Please reread the comment above and you will find it there. If you are looking for a reduced test case then cause any error, which I know you know what these are, and check the result.code it will be undefined in v3.x and in v2.x it will be the untouched code.

As I stated up here you either have the docs incomplete (which is highly possible and previously proven) or you forgot to return the untouched code on error with minify(). e.g. a regression or a documentation error.

Please do us all a favor and reread until you understand the issue here. As I stated before I personally won't bother with a PR (this includes test cases for Travis CI and/or AppVeyor) since you seem to have issues with how the nodejs ecosystem currently works. However bug reports from OUJS as a whole with the backing of thousands of users would appreciate some effort from this project as we offer that same, and already submitted, consideration already.

Thank you for rereading the issue and all of its comments including PR (comments) posted to this issue.

---

1934 is a start albeit incomplete with the migration. I can't guess what your intentions are... that's why your team is here... is it a regression or is it a undocumented migration feature?

---

Here is the official OUJS work-around summary code, without much reading, at OpenUserJs/OpenUserJS.org/blob/c38c51e/controllers/scriptStorage.js#L754-L769. This is precisely what you are asking with:

... only involves uglify-js/uglify-es.

e.g. it doesn't get any more explicit than this.

[rvanvelzen]

I can't reproduce not getting an error field in the response object. - with the given source:

{ error: 
   { [SyntaxError: Unexpected token: punc (})]
     message: 'Unexpected token: punc (})',
     filename: '0',
     line: 269,
     col: 0,
     pos: 8141 } }

@Martii There is absolutely no need to talk down to anyone here. You really haven't explained your problem clearly.

[Martii]

@rvanvelzen

@Martii There is absolutely no need to talk down to anyone here.

If I was "talking down" everyone would know it. Please don't read more into this than what is stated. This is a clear, simple, fully documented, explanation.

You really haven't explained your problem clearly.

See prior response in this comment. e.g. I have... even edited... replies afterwards.

I don't see any changes between 3.0.4 and 3.0.5 that affect this but when an error comes through specific to the gist example that I posted with minify() it does, in fact, not return return.code as the original code since that is probably an errored Userscript.

What we need is a definitive answer on is it...

1. A regression where the .code was forgotten to be returned? :question:
2. Is it a new feature where it doesn't return .code if there are errors? :question:

I have asked this question set multiple times now.

with the given source:

That is after the workaround... but try the source above in the original comment where the issue template asks "The uglifyjs CLI command executed or minify() options used."

[rvanvelzen]

Is it a new feature where it doesn't return .code if there are errors? :question:

I don't know how this would be new - previously we would throw an error so you could never have retrieved a code property.

Returning the error instead of throwing it was introduced in a3b2eb75bd57e78305c418242c538c41acf010e7 (which appears first in in 3.0.5).

[Martii]

previously we would throw an error so you could never have retrieved a code property.

So it's probably a bug fix from v2.x to v3.x then. Even express-minify relies on the v2.x behavior which is why I am asking because when it gets upgraded to v3.x they'll (probably) have to make a modification. (since the error is never thrown but only captured at .error now. Ref: #1880 .. thank you)

Returning the error instead of throwing it was introduced in a3b2eb7 (which appears first in in 3.0.5).

And this implies new feature. :\ It's also showing up in 3.0.3 and 3.0.4 which is why this issue was opened... not sure what's up there (rebase issue?... tag 3.0.5 vs pre-release issue with 3.0.1?). We haven't done 3.0.5 yet as that was released a few hours ago for ES5 and not ES6+ yet on npmjs.com... but did test harmony briefly in the last 24 hours.

INSERTED See also:

• breeswish/express-minify/blob/970b003/minifier.js#L58
• breeswish/express-minify/blob/970b003/minifier.js#L159
• @breeswish this is going to make breeswish/express-minify#42 a little shaky between ES5 releases (master and *uglify-js package)* and ES6+ (harmony and *uglify-es package)* ... the trap pattern we use now might handle both and you are welcome to adapt some variant of that if needed

\/INSERTED

We've been getting .code, uncompressed/untouched, for years now with errors as documented at OpenUserJs/OpenUserJS.org#432 (with harmony on OUJS).

---

Apologies for the bazillion edits in this reply but trying to ensure this is crystal clear for everyone.

[alexlamsl]

result.error from the new minify() is documented here:

https://github.com/mishoo/UglifyJS2#api-reference

[Martii]

@alexlamsl

https://github.com/mishoo/UglifyJS2#api-reference

I know that. That is how I figured out the workaround... but the migration from v2.x to v3.x is undocumented... and PR's and commits don't count as everyone else has expressed with having a CHANGELOG, migration guide, or at the very least a proper tag as mentioned over at #1881. Respectfully I would appreciate it if you didn't respond from now on to any issue.

@rvanvelzen
You really need to control @alexlamsl from being closing happy. As I stated before he's going to drive the project into the ground. This is why I will never submit a PR personally again. He's going to be driving more contributors away. This is very bad maintenance and I'm not the only one that thinks this (proof positive at #1875). He can't seem to understand what's going on here when it's beyond a doubt an issue.

[rvanvelzen]

@Martii @alexlamsl is the primary maintainer. I do not share your concerns, as he's doing a great job.

[Martii]

Your one vote doesn't supersede the visible 54 bad votes. I have no doubt that his coding skill is quite well however his presence could use major improvement. :)

Anyhow... you still have confused others around the office with your in-congruent statements but we'll go with the #1880 and the fact that you guys want PRs but then treat issuers badly for now since even you were clearly confused up here.

---

Current favorite subject here of:

keep minify() options in sync

Had to actually open that PR to figure out it was grunt based... so this project wants to generate more click through traffic and let everyone else try to figure out what's been done rather than a proper CHANGELOG and MIGRATION GUIDE to curtail additional noise. Seems lazy and irresponsible to us :wink: