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

Fix output of compiled coffeescript applications #85

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
7 participants
@dsummersl

dsummersl commented Apr 22, 2012

The compiler was wrapping each javascript file compiled from coffee script with a function, which prevents any methods/variables from being used by other coffee scripts. I fixed this by passing the 'bare' option to the compiler.

Prevent the compiler from wrapping the script in function wrapper - s…
…o that separate scripts can see each other's defs
@debergalis

This comment has been minimized.

Member

debergalis commented Apr 24, 2012

+1 We'll take it. See https://github.com/meteor/meteor/wiki for guidelines on basing against devel and signing the CLA.

Duplicate of #33.

@dsummersl

This comment has been minimized.

dsummersl commented Apr 25, 2012

signed.

@debergalis

This comment has been minimized.

Member

debergalis commented Apr 25, 2012

078cdaf thanks (i took the #33 patch)

@debergalis debergalis closed this Apr 25, 2012

@jlfwong

This comment has been minimized.

jlfwong commented Apr 25, 2012

CoffeeScript defaults to not using the bare option for a reason. If you want things to be exported, you have to explicitly say so by attaching them - either with @foo = foo or window.foo = foo if you're dealing with browsers only.

Having module level scope by default in CoffeeScript is useful, and as a CoffeeScript user, it's what I'd expect.

@dsummersl

This comment has been minimized.

dsummersl commented Apr 25, 2012

I suppose its a matter of opinion, imo: do you expect the js/coffee equivalent meteor app to work the same way out of the box, or do you need to conform to the coffee standard (non-bare). I'm a coffee user too, and former spine user: I prefer the module.export/require(xxx) (explicit export, explicit import)...but it seems thats not the meteor standard wrt js, and I personally think the js/coffee treatment should be the same.

@holmsand

This comment has been minimized.

holmsand commented Apr 25, 2012

I think it would be very confusing for Meteor to have its own CoffeeScript dialect (and incredibly error prone).

File level scope is quite a fundamental part of plain old CoffeeScript (see http://coffeescript.org/#lexical_scope). As far as I know, the "bare" option is mainly meant for e.g. Node's module loader, that already does its own wrapping-in-anonymous-function stuff.

@dsummersl

This comment has been minimized.

dsummersl commented Apr 25, 2012

Its not a 'new' dialect - its a dialect already supported by coffeescript. I understand the scoping feature but I think that meteor's coffeescript should be handled the same way that its js is handled.

If meteor supports explicit export/import. I DO prefer explicit export/imports via something like CommonJS. Then we wouldn't see questions about the order in which js/coffee files are loaded...

Maybe rather than arguing about whether the --bare option is bad/good, we can talk about getting actual dependencies on the client side js/coffee?

@holmsand

This comment has been minimized.

holmsand commented Apr 25, 2012

Why do you say that this "is a dialect already supported by coffeescript"? According to the documentation I linked to it most definitely is not.

You can of course argue that CoffeeScript should work differently, but I believe that forcing a non-standard behavior on everyone is wrong.

@jlfwong

This comment has been minimized.

jlfwong commented Apr 25, 2012

The key issue is this:

Which is more important? That coffeescript users learning meteor have things work as expected, or that meteor users learning coffeescript have things work as expected?

I suspect that far more coffeescript users will be learning meteor than the other way around, so I think that bare should not be the default.

As for CommonJS dependencies being built-in, I do think that's a good path to take, but it does introduce the necessity of more code.

As an aside, for whatever reason, the node require system and CommonJS are not technically compatible. There are tiny details that make node package.json's not valid CommonJS package.json's.

@jussiry

This comment has been minimized.

jussiry commented Jun 3, 2012

I strongly oppose the current bare: true option. This is against the principles of CoffeeScript and brings back one of the most notorious features of Javascript: exposing new variables in global namespace.

In general, it's hard for me to see why we should change the way CoffeeScript works in Meteor? Javascript should work like Javascript and CoffeeScript should work like CoffeeScript, independent of the environment.

@jussiry

This comment has been minimized.

jussiry commented Jun 3, 2012

@dsummersl, when you wan't to make a global var, simply use @some_global_var = 123.

@thurn

This comment has been minimized.

thurn commented Apr 30, 2013

I agree with @jussiry and @phleet, this was a poor decision. The CoffeeScript documentation states "all CoffeeScript output is wrapped in an anonymous function". It is extremely surprising that the meteor CoffeeScript package has different behavior (and AFAIK no way to override this setting)... why would I request coffeescript for my application but want non-standard behavior? The fact that this encourages people to write bad code that pollutes the global namespace is just a secondary concern after that.

@awwx

This comment has been minimized.

Contributor

awwx commented Apr 30, 2013

Actually the CoffeeScript output is now wrapped in an anonymous function by the packaging system, so there's no need for the CoffeeScript output to itself include the anonymous function wrapper (you end up with the same thing).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment