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

Fixed: added semicolon at the end of the file #17

Closed
wants to merge 1 commit into from
Closed

Fixed: added semicolon at the end of the file #17

wants to merge 1 commit into from

Conversation

pavelivanov
Copy link

Crash concatenate js files

@nolanlawson
Copy link
Member

This dist file is actually auto-generated, so it's browserify that decided not to put a ; at the end...

@mmrko
Copy link

mmrko commented Jan 13, 2015

I came across this issue as well a while back. It did take some time to track it down. Bumping browserify to 5.0.0 would fix the semicolon issue but also require changes to the build. If I find the time at some point I can look into this better. For now, I'm just using the minified version. Changing bower.json to point to the minified version could save people from the possible headache. Thanks for the awesome plugin btw! :)

@nolanlawson
Copy link
Member

What concatenators out there aren't adding \n or ; between files? I've got lots of plugins using this version of Browserify, so I'm loathe to go out and fix them all. >_<

@mmrko
Copy link

mmrko commented Jan 13, 2015

Well, this is definitely somewhat of an edge case. I'm using gulp-useref, which in turn uses gulp-concat that passes \n as the separator for concat-with-sourcemaps. In some scenarios a newline doesn't however guard against the missing semicolon issue.

@nolanlawson
Copy link
Member

TBH I'm of the opinion that this is a bug in gulp-concat. Much easier to fix it in one build tool, as opposed to fixing it in every JS lib on the planet. :)

Sorry to be a bit stubborn, but I'm gonna close this one.

@nolanlawson
Copy link
Member

I filed an issue on gulp-concat ^

@mmrko
Copy link

mmrko commented Jan 13, 2015

Yup, I agree :)

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

Successfully merging this pull request may close these issues.

3 participants