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

Strange bug with bare option #2

Closed
jescalan opened this issue Dec 14, 2013 · 17 comments
Closed

Strange bug with bare option #2

jescalan opened this issue Dec 14, 2013 · 17 comments
Assignees
Labels

Comments

@jescalan
Copy link

This is super weird, and I'm not totally sure it's gulp-coffee's fault, but hear me out on this one.

Gulp task, compiles coffeescript. I add in the { bare: true } option, assuming all will go well since I'm compiling node anyway. When they come out, the coffeescript extension __extends is not getting compiled at the top of files that use classes (this is breaking and causes all files with classes to immediately throw an error).

Get rid of the bare option and it's back, albeit with an extra useless function wrapper now. Looks like a bug in coffeescript to me, really. Make sure locally installed coffeescript is the same version as the one required by gulp-coffee (1.6.3, yep, but you guys should be using 1.x.x in your package.json, because semver), compile the file manually with the bare option. __extends is defined in the files, and no function wrapper, everything works perfectly. Wat?

So I checked the source and it looks like you guys are just compiling through coffee-script, as usual, but I figured I'd at least log this here because compiling with coffee manually didn't break anything, but the compilation through gulp caused breakage.

Any thoughts or help on this appreciated!

@yocontra
Copy link
Member

Interesting. Will look at this when I have a minute.

@ghost ghost assigned yocontra Dec 28, 2013
@leny
Copy link

leny commented Jan 5, 2014

Additionally, when you use { bare: true }, the coffee-script compiler doesn't declare top-scope variables anymore... and strips all the empty lines.

@leny
Copy link

leny commented Jan 5, 2014

After some tests, the bug seems happening when giving multiple files by glob.

gulp.src( [ "src/**/*.litcoffee" ] )
        .pipe( coffee( options ) )
            .on( "error", gutil.log )
        .pipe gulp.dest "bin"

-> generates bugs and undeclared variables

gulp.src( [ "src/models/dicecheck.litcoffee" ] )
        .pipe( coffee( options ) )
            .on( "error", gutil.log )
        .pipe gulp.dest "bin"

-> generate proper compiled file


I have absolutely no idea how to debug this (I still haven't much experience with gulp), but this is really anoying... :(

@leny
Copy link

leny commented Jan 5, 2014

I've found it !

When you send multiple files, the optionsseems to be populated with properties which confuse the coffeescript compiler.

I will make a pull request this afternoon.

@yocontra
Copy link
Member

yocontra commented Jan 5, 2014

Which options?

Also, does the litcoffee stuff work fine without telling the coffee compiler specifically that it is litcoffee?

@yocontra
Copy link
Member

yocontra commented Jan 5, 2014

Hmm maybe we should infer that from the file extension instead of explicity passing in literate: true - thoughts?

@leny
Copy link

leny commented Jan 5, 2014

When you don't give literate: true, compilation fails on some files :/

dashed added a commit to dashed/gulp-coffee-sandbox that referenced this issue Jan 5, 2014
@dashed
Copy link
Contributor

dashed commented Jan 5, 2014

I can confirm this bug for config { bare: true }.

Using the class example in http://coffeescript.org/#classes

The compiled js gets borked: https://github.com/Dashed/gulp-coffee-sandbox/blob/master/src/subdir/a.js

The global vars seems to be missing.

@leny
Copy link

leny commented Jan 5, 2014

I'm currently writing a fix, I will make the pull request soon.

@leny
Copy link

leny commented Jan 5, 2014

Et voilà ! 290f1fc

@dashed
Copy link
Contributor

dashed commented Jan 5, 2014

Fantastic, I just tested your PR on my sandbox. Works perfectly.

@leny
Copy link

leny commented Jan 5, 2014

You're welcome.

I hope it will be merged & published on npm soon, to fix the issues in my work-project builds... :)

@dashed
Copy link
Contributor

dashed commented Jan 5, 2014

@leny You may want to add more default options into your PR as listed here: https://github.com/gruntjs/grunt-contrib-coffee/blob/master/tasks/coffee.js#L17

@leny
Copy link

leny commented Jan 5, 2014

@dashed done. Thanks.

@jescalan
Copy link
Author

jescalan commented Jan 5, 2014

👍 great work guys!

@dashed
Copy link
Contributor

dashed commented Jan 5, 2014

@Jenius it's patched in 1.2.5 and is available on npm. =] if it works as your side, and there are no outstanding problems, then please close this ticket

@jescalan
Copy link
Author

jescalan commented Jan 5, 2014

Looks good - thanks guys!

@jescalan jescalan closed this as completed Jan 5, 2014
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

4 participants