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

Minifying angular-bootstrap-datetimepicker fails with gulp-uglify #338

Closed
henrikthorin opened this issue Sep 12, 2016 · 13 comments
Closed
Labels

Comments

@henrikthorin
Copy link

henrikthorin commented Sep 12, 2016

Since your last update where you have removed all semicolons in your code, our gulp-minify fails resulting in a corrupt minified js file.

Please advice us what do to.
We will revert to 1.0.1 until we have a working solution.

@rbengtsson
Copy link

We're having this issue as well!

@dalelotts
Copy link
Owner

Can you create a Reduced Test Case/example for this problem?

Perhaps a minimal gibhub project that shows the problem.

You MUST have a recreation or I cannot work on this issue.

@amunro
Copy link

amunro commented Sep 12, 2016

Absolutely running into this issue.

@dalelotts
Copy link
Owner

dalelotts commented Sep 12, 2016

Still checking on a solution...

@dalelotts
Copy link
Owner

dalelotts commented Sep 12, 2016

When concatenating the files together, adding in a semicolon between the file solves the issue. I've manually tested this several different ways, using multiple semicolons and various combinations of \r, \n, and ; and it always works after minification.

gulp.task('minify', function () {
  var destination = path.join(__dirname, 'build', 'js')
  return gulp.src(paths.src)
    .pipe(concat('datetimepicker.all.min.js', {newLine: ';'}))
    .pipe(uglify())
    .pipe(gulp.dest(destination))
});

This is the line that solves the problem .pipe(concat('datetimepicker.all.js', {newLine: ';'}))

@dalelotts
Copy link
Owner

dalelotts commented Sep 12, 2016

This also works - changing the order of the operations and uglify first, then concatenate will put each file on its own line separated by \n. I guess is just depends on which output you like.

gulp.task('minify', function () {
  var destination = path.join(__dirname, 'build', 'js')
  return gulp.src(paths.src)
    .pipe(uglify())
    .pipe(concat('datetimepicker.all.min.js'))
    .pipe(gulp.dest(destination))
});

@dalelotts
Copy link
Owner

I forgot one very important rule about ASI in JavaScript. Never start a line with (, [, or `

I'll release a fix shortly.

For the following releases (not this one!), I will update the tests so they also run against the minified code so I can be 100% sure the code will minifiy correctly before release.

@dalelotts
Copy link
Owner

Thanks for your patience and understanding @HONK82 , @rbengtsson , @amunro, and everyone else.

I should know better! 💯% of the time when I think the problem is not in my code...the problem is in my code! 😞

@amunro
Copy link

amunro commented Sep 12, 2016

Appreciate the updates! If I had $1 for every bug i created.... I'm just glad it wasn't me this time :)

@henrikthorin
Copy link
Author

Thank you for fixing this issue.

@dalelotts
Copy link
Owner

@HONK82 Thanks for reporting it!

dalelotts added a commit that referenced this issue Oct 21, 2016
Some users have reported that their build fails when concatenating this file with other files that

use IIFE's. Having a semi-colon before and after the IIFE's in this code will allow the users build

system to continue to work as before.

Fixes #347 and #338
@calopez
Copy link

calopez commented Dec 15, 2016

This fix is not included in the last version(1.1.3) of the library.

I guess just missing a new tag 1.1.4 for the current master

@dalelotts
Copy link
Owner

dalelotts commented Dec 16, 2016

@calopez I just downloaded the source - if this is not working for you then you likely have a defect in your build process.

See #347 for options to fix your build process.

dalelotts added a commit that referenced this issue Mar 31, 2017
Calling this a fix so a patch is released. Update dependencies and fix failin 中文 tests. 對不起我的中文不好

See https://www.youtube.com/watch?v=2XTBwvi0h2E

Fix #338
dalelotts added a commit that referenced this issue Mar 31, 2017
Update dependencies and fix failing tests that resulted from moment updating the 中文 time format yet

again. 對不起我的中文不好 See https://youtu.be/2XTBwvi0h2E

Fix #338
dalelotts added a commit that referenced this issue Mar 31, 2017
Update dependencies and fix failing tests that resulted from moment updating the 中文 time format yet

again. 對不起我的中文不好 See https://youtu.be/2XTBwvi0h2E

Fix #338
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants