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

Namespace issue #170

Closed
Sivli-Embir opened this issue Jan 22, 2016 · 14 comments
Closed

Namespace issue #170

Sivli-Embir opened this issue Jan 22, 2016 · 14 comments
Labels

Comments

@Sivli-Embir
Copy link

I have a collection named "Route". After a lot of debugging "TypeError: object is not a function" I realized my collection was being used by Restivus in place of its internal Route class. I already check, your not exporting Route and it is not available in global scope. Yet by changing three lines of your code I made "Route" -> "Restivus.Route". This fixed my issue, likely a global scope problem.

I can continue to consume your package locally, version locked to 0.8.3, but wondered if you had any suggestions.

@kahmali
Copy link
Owner

kahmali commented Jan 25, 2016

Sorry about that. Not sure why that's happening when those variables shouldn't be exposed to the global scope. I'm happy to accept an update for this. Do you want to submit a pull request with the necessary changes? Please just read the guidelines before submitting.

@Sivli-Embir
Copy link
Author

Will do, how do you want it Namespaced? I did Restivus.Route for ease but anything would work really.

@kahmali
Copy link
Owner

kahmali commented Jan 25, 2016

Hmmm. Good question. I don't want to expose it outside the package if I can avoid it. Restivus is exported, so my first thought is that we shouldn't put it there. Gimme a sec. It's been a while since I've actually looked at the Restivus code :P

@Sivli-Embir
Copy link
Author

See https://github.com/Kestanous/meteor-restivus if you want to see what I did, still reviewing your guidelines so it may need some work.

@Sivli-Embir
Copy link
Author

After some thought I think it may be a coffeescript issue. I think package top level this "@" is actually app global. I recall having an issue with this a long time ago. I believe that is one of the reasons I now only write packages in js vs coffee, and keep coffee for apps only.

@kahmali
Copy link
Owner

kahmali commented Jan 25, 2016

I was just looking into that. I think you're correct. But the meteor docs suggest there may be some fixes easier than converting the entire package to js. From the meteor docs on CoffeeScript:

There is no way to make a package-scope variable from a .coffee file other than exporting it. We couldn't figure out a way to make this fit naturally inside the CoffeeScript language. If you want to use package-scope variables with CoffeeScript, one way is to make a short .js file that declares all of your package-scope variables. They can then be used and assigned to from .coffee files.

If you want to share variables between .coffee files in the same package, and don't want to separately declare them in a .js file, we have an experimental feature that you may like. An object called share is visible in CoffeeScript code and is shared across all .coffee files in the same package. So, you can write share.foo for a value that is shared between all CoffeeScript code in a package, but doesn't escape that package.

They've been claiming that feature to be experimental for over a year now, and they still haven't removed it from the docs, so folks must have not complained about it being broken. We could try that as well. I think I prefer the first option though. Not a huge fan of having this random share variable floating around code. Any preference? Do you mind creating a lib/package-scope-variables.js file and testing this approach? Feel free to name it something different if you think of anything less verbose.

@Sivli-Embir
Copy link
Author

Hmm yeah, I will give the first a shot. It would be nice to solve this.

@kahmali
Copy link
Owner

kahmali commented Jan 25, 2016

Awesome. Keep me posted. Thanks for following up on this!

@Sivli-Embir
Copy link
Author

So as far as I can tell option one does not work, we would have to define the classes in the package-scope-variables.js file. Which we can't do if you want to use Coffeescript classes. Defining the class as one of those vars in another file is still file scoped, Route = {} will be true for all other files. Maybe I am doing it wrong but I don't think so.

Option two could work as an alternative but at that point it is the same as my solution (and I think mine is cleaner).

At this stage I need to ask why. Why don't you want the Route class to be available externally, though hidden under some scope (e.g. Restivus).

@kahmali
Copy link
Owner

kahmali commented Jan 25, 2016

Restivus isn't a namespace right now. It's a class. It feels weird to me to nest one class under another like that.

@Sivli-Embir
Copy link
Author

Fair point, though IMO an object is an object. We could use the magic shard object instead. It is semantics really, so whatever your comfortable with.

One thing to note is your tests prove Route is on the Global scope as they can access it directly. If we fix this you are going to need a way for your tests to find that object. I proved that when testing option one. I think we can add the lib/route.coffee in the onTest function as a fix but I am not sure. All around this scoping is a bit of a headache.

@kahmali
Copy link
Owner

kahmali commented Jan 25, 2016

The other option is to introduce another "namespace" that will behave exactly like Route does now, and may interfere in other projects that are currently using Restivus. So I'd rather work with the share object. I just tested it locally and it works as expected, without any need to change the API. You just have to add the file in the test definition as well to get the tests to pass, which is certainly less than ideal, but OK for now. I'll have to reconsider how the tests are setup at some point, but that's not any worse than what's happening now (with the global namespace being polluted). I agree that this is an extremely annoying issue with CoffeeScript in Meteor packages.

@kahmali
Copy link
Owner

kahmali commented Feb 24, 2016

I just published v0.8.5 with the fix for this. Let me know if you have any issues with that.

@kahmali kahmali closed this as completed Feb 24, 2016
@kahmali kahmali added the bug label Feb 24, 2016
@Sivli-Embir
Copy link
Author

Thank you, sorry lost track of this. We are in production now with my quick fix so it may be a while before I get to testing this in the wild.

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

2 participants