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

Proposal: simple support for psuedo-namespaces #878

Closed
nkohari opened this issue Nov 27, 2010 · 4 comments
Closed

Proposal: simple support for psuedo-namespaces #878

nkohari opened this issue Nov 27, 2010 · 4 comments

Comments

@nkohari
Copy link

nkohari commented Nov 27, 2010

When using JS frameworks, it's a common practice to create psuedo-namespaces for classes by creating a tree of dummy objects. For example:

A = {}
A.B = {}
A.B.SomeClass = function() { ... }

While CoffeeScript technically supports this behavior, it names classes based on the last property access for the variable used in a class definition. For example, if I have a class named A.B.SomeClass, it will result in the following JavaScript to be generated:

A.B.SomeClass = SomeClass = function() { ... }

What I propose is that CoffeeScript instead name classes based on a concatenation of the property names used in the class definition. That is, for the same class A.B.SomeClass, CS would generate:

A.B.SomeClass = A_B_SomeClass = function() { ... }

This would ensure that if SomeClass was also defined in another namespace, the two classes would not conflict. For classes not using psuedo-namespaces, their names would be unchanged.

I believe this requires a simple change to determineName in the Class node. I've implemented a working version on my fork at: nkohari/coffee-script@bff2261af7045010502c7d68d1a0da3a6ff5ecf3

Any thoughts on the value of this?

Edit: noticed that my implementation clobbers the names of classes like Rewriter. So that's probably a bad thing. :)

@nkohari
Copy link
Author

nkohari commented Nov 27, 2010

Another possibility discussed in IRC is that the output of:

class A.B.SomeClass extends D

could be identical to the output of this syntax:

A.B.SomeClass = class extends D

In this case, SomeClass is not defined in the top-level function. This is much more correct, although I'm not quite certain how to implement the patch for it. :)

@jashkenas
Copy link
Owner

How do the names conflict? Aren't they only referenced within the local closure? Are you talking about constructor.name returning the same string for the two different classes?

@nkohari
Copy link
Author

nkohari commented Nov 28, 2010

You're right, they are only referenced within the closure. However, if I create a class called SomeNamespace.Collection, it will also be defined as Collection within the closure. If a user of the framework then creates a class called AnotherNamespace.Collection, inclusion order will determine which class will claim the Collection reference in the top-level closure.

It's definitely not a major problem (I'm using namespaced types right now with no issues), but I can see the potential for problems in certain edge cases. After tinkering with the CS source a bit more it does look like the second solution (in the comment after the OP) is the most correct one.

@jashkenas
Copy link
Owner

Thanks for opening the ticket -- inner class names should have been used to name the function, but should not have been leaking to outer scope. Here's a patch that fixes the issue, with a test case. Please give it a read-over if you have a minute:

1254efa

Closing the ticket...

stephank pushed a commit to stephank/coffee-script that referenced this issue May 14, 2011
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants