googlearchive / polybuild

Easy Optimization Tool / Gulp plugin
BSD 3-Clause "New" or "Revised" License
75 stars 19 forks source link

uglify breaks firebase.js #27

Open joncostello opened 8 years ago

joncostello commented 8 years ago

Moved here from: https://github.com/PolymerElements/polymer-starter-kit/issues/378

Steps to reproduce:

yo polymer yo polymer:el test-firebase bower install --save GoogleWebComponents/firebase-element

Insert a test-firebase tag in index.html(I put it above the my-greeting tag).

Inside test-firebase.html: link firebase-document above hello from test-firebase put a firebase-document tag

gulp serve:dist Result: blank page. Dev console has the Uncaught SyntaxError: Unexpected token ILLEGAL

zboralski commented 8 years ago

Same problem here...

<script language="JavaScript" type="text/javascript" src="../../bower_components/jsrsasign/jsrsasign-latest-all-min.js"></script>

with maximumCrush='true' I get :

Message:
    Expecting UnicodeEscapeSequence -- uXXXX

and maximumCrush='false' :

Message:
    Line 240: Unexpected token ILLEGAL
Details:
    index: 258234
    lineNumber: 240
    column: 2902
    description: Unexpected token ILLEGAL

I am using the following vulcanize task from the mobile app recipe:

// load polybuild
var polybuild = require('polybuild');

// Vulcanize granular configuration
gulp.task('vulcanize', function() {
  return gulp.src('app/elements/elements.html')
    .pipe(polybuild({maximumCrush: true}))
    .pipe($.rename(function(file) {
      if (file.extname === '.html') {
        file.basename = file.basename.replace('.build', '');
      }
    }))
    .pipe(gulp.dest(dist('elements')))
    .pipe($.size({title: 'vulcanize'}));
});
zboralski commented 8 years ago

I tried creating a non-minified version of jsrsasign (polybuild only choke on the minified version) but it is a cross-dependency nightmare and gave up.

zboralski commented 8 years ago

This seems to be related :

https://github.com/mishoo/UglifyJS2/issues/544

zboralski commented 8 years ago

I don't think uglify is the problem.... If I comment out this line :

  .pipe(crush ? uglify : leftAlign)

polybuild succeeds... but then my application crashes at runtime with the same error:

So I run HTMLPrettify on elements.build.js to see what line was causing the problem :

elements.build.js:9213 Uncaught SyntaxError: Unexpected token ILLEGAL
9213:   if (f--\x3e0) {

What I don't understand is where that \x3e0 comes from because the code in question is jsbn.js(http://www-cs-students.stanford.edu/~tjw/jsbn/)

and the original line is if (f-- > 0) { ... and the non-minified version is if(i-- > 0) {

zboralski commented 8 years ago

The problem is with vulcanize....

I ran vulcanize --inline-scripts app/elements/elements.html

and that f (f-- > 0) was changed to if (f--\x3e0)

mm-gmbd commented 8 years ago

+1, same issue

EDIT: So, my problem appears to be different than @zboralski has explained, as mine is with firebase.js speficially.

After running Atom Beautify on firebase.js (firebase.js installed through bower), I was able to track down the problem (I think). On line 4716 of the source, I came across the following line:

this.Ga.src && "javascript:" === this.Ga.src.substr(0, 11) && (a = '<script>document.domain="' + document.domain + '";\x3c/script>');

However, after running through Polybuild, the resulting line in index.build.js is the following:

this.Ga.src && "javascript:" === this.Ga.src.substr(0, 11) && (t = '<script>document.domain="' + document.domain + '";

Notice the end of the line -- there is no \x3c/script> or </script>, and the string is not terminated! Not sure what tool under the hood is causing the issue, but that appears the be the issue.

EDIT 2: After manually changing the resulting index.build.js so that the above line is corrected, I receive a different error:

Uncaught SyntaxError: Unexpected end of input

It is the last line of the file, and I'm guessing because of the error above, the rest of the file is parsed incorrectly and therefore produces a complete dud.

spirylics commented 8 years ago

+1 firebase + polybuild (uglify) broken

justinfagnani commented 8 years ago

This was fixed in vulcanize 1.14.7

mm-gmbd commented 8 years ago

@justinfagnani, just ran again and have the same issue. Has the dependency for vulcanize been updated for polybuild?

justinfagnani commented 8 years ago

No, but a fresh npm install should pick up the latest version.

mm-gmbd commented 8 years ago

npm uninstall polybuild followed by npm install polybuild results in the same...

justinfagnani commented 8 years ago

argh, npm is terrible :(

The constraints from polybuild -> gulp-vulcanize -> vulcanize should pick up 1.14.7

I'll have to file a request over at gulp-vulcanize to update their constraints.

justinfagnani commented 8 years ago

Filed https://github.com/sindresorhus/gulp-vulcanize/issues/36

mm-gmbd commented 8 years ago

Awesome! Mind re-opening until that is updated considering Polybuild is still afflicted?

justinfagnani commented 8 years ago

Sure thing

mm-gmbd commented 8 years ago

Also, just to make sure I'm not crazy, I'll manually update gulp-vulcanize in my own node_modules folder and see if it indeed works.

justinfagnani commented 8 years ago

Make sure you're picking up vulcanize 1.14.7 in there

mm-gmbd commented 8 years ago

So, I made sure that vulcanize in my node_modules folder was v1.14.7 (and gulp-vulcanize simply does require('vulcanize'), so it should be the right version). But, the resulting files are still broken, and they are broken the same way as before (as described in my first comment above).

samcarecho commented 8 years ago

Hi.

Any good news about the issue?

I've tried to use the follow code to remove firebase.js from being vulcanized, but it's not removing it.

gulp.task('vulcanize', function () {
  return gulp.src('dist/index.html')
    .pipe($.vulcanize({
      stripComments: true,
      inlineCss: true,
      inlineScripts: true,
      excludes: [path.resolve('./dist/bower_components/firebase/firebase.js')]
    }))
    .pipe(polybuild({maximumCrush: false}))
    .pipe(gulp.dest('dist/'));
});

How can I properly avoid firebase.js from being vulcanized?

spirylics commented 8 years ago

Hi,

It's not very clean, but it works, in waiting :

<script async>
            var script = document.createElement('script');
            script.async = true;
            script.src = 'firebase.js';
            document.head.appendChild(script);
</script>
mm-gmbd commented 8 years ago

@spirylics, it looks like you have an extra set of closing brackets/parenthesis });, just wondering if this is an accidental addition or an accidental omission of something from the beginning.

spirylics commented 8 years ago

@mm-gmbd , It's an accidental addition :) Thanks

evancaldwell commented 8 years ago

I'm still having an issue with this. I'm on vulcanize 1.14.8 and gulp-vulcanize 6.1.0. I'm not sure what to do with @spirylics snippet above.

mm-gmbd commented 8 years ago

@evancaldwell, I actually meant to follow up on this and never did. @spirylics, does the script you provided just get inserted somewhere in the index.html? I presume that it's position is important, but I'm with @evancaldwell and am not really sure (also, I haven't tested -- sorry!)

spirylics commented 8 years ago

This script posted above will replace your firebase script declaration. Like that uglify don't see firebase js and don't touch it !

evancaldwell commented 8 years ago

hmm.. @spirylics maybe my setup is different, I'm using Firebase polymer elements so the firebase script declaration is actually in bower_components/firebase-element/firebase.html

spirylics commented 8 years ago

Ok I see. In your case you can use importHref from Polymer utility fonctions : https://www.polymer-project.org/1.0/docs/devguide/utility-functions.html

The idea is to import dynamically firebase (script or html) to hide it of vulcanize / uglify...

evancaldwell commented 8 years ago

@spirylics sorry for my noobness, a couple further questions: should I be using importHref in my index.html? Also won't I still get an error after everything is vulcanized because I'm still importing the firebase elements that are causing the error?

@samcarecho were you ever able to fix up the gulp task to exclude firebase.js?

spirylics commented 8 years ago

Yes you can use importHref in your index.html. Just, you have to use firebase-element when firebase-element.html is loaded ; so use onload argument of importHref.

samcarecho commented 8 years ago

@evancaldwell the project on which I was working is using polybuild. I did not manage to exclude firebase from it.

dChin84 commented 7 years ago

@mm-gmbd I am experiencing the same issue - Invalid or unexpected token when vulcanized. I have updated the version of vulcanize etc. Did you manage to resolve?

mm-gmbd commented 7 years ago

@dChin84 - unfortunately, I never resolved this on my end. I couldn't track down the underlying tool that was causing the parse error, so I ended up just leaving. However, it's been so long now that Firebase has released an entirely new set of libraries/SDKs (v3), does that mean this issue persists into v3 (or are you still using a v2 lib)?

dChin84 commented 7 years ago

@mm-gmbd Thanks for your response. It's looking like the issue is still present I am using v3 of the SDK :(

samcarecho commented 7 years ago

OMG,

Is this still an issue? Can´t believe no one from Google is capable of fixing it! :O

On Tue, Dec 13, 2016 at 5:53 PM, Daniel Chin notifications@github.com wrote:

@mm-gmbd https://github.com/mm-gmbd Thanks for your response. It's looking like the issue is still present I am using v3 of the SDK :(

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/PolymerLabs/polybuild/issues/27#issuecomment-266794074, or mute the thread https://github.com/notifications/unsubscribe-auth/AAyrA3CQuwLoY4u8MWuIr9Se9CmjaHw5ks5rHs2BgaJpZM4HKowT .

mm-gmbd commented 7 years ago

@samcarecho - I'm with you. This should have been fixed a long time ago, especially considering that both of these are Google products/projects...

dChin84 commented 7 years ago

I managed to work around the issue by modifying polymerfire removing the references to firebase so they don't get vulcanized just allowing polymerfire to be vulced and then including firebase as script references within the main page. Google please fix as this is far from a decent solution

fernandezpaco commented 7 years ago

Any plans to solve this issue? It is affecting production

Thanks

justinfagnani commented 7 years ago

@fernandezpaco we've removed uglify in favor of Babli from Polymer CLI. That change will be out in the next release. If you're still using polybuild, we recommend switching to the CLI or the polymer-build library, which is what powers the CLI.

jariaspe commented 7 years ago

Hi @justinfagnani,

I work with @fernandezpaco and we've made a POC including the rxjs.lite.min.js in a new app created from scratch with Polymer-cli. We just include the script in my-app.html.

We've included a gulp task to build it as it's explained in generator-polymer-init-custom-build.

We still have the same problem that @zboralski reported. Inside rxjs.lite.min.js there's a this._r-->0 which is replaced by this._r--\x3e0 and breaks the execution.

In the current vulcanize task (now renamed to polymer-bundler) we could narrow the problem down to the Uglify2JS run by inlineScripts in this line.