search

gadicc / meteor-headers

Access HTTP headers on both server and client. Client IP with proxy support.
https://atmospherejs.com/gadicohen/headers
GNU Lesser General Public License v3.0
61 stars 21 forks source link

tinytest fails #17

Closed Nemo64 closed 10 years ago

Nemo64 commented 10 years ago

I tried to use this package in one I'm developing right now. The thing is, when used inside tinytest, it fails with Uncaught ReferenceError: __headers__ is not defined. Can you think of why?

gadicc commented 10 years ago

Uh, I actually have no experience with tinytest, but I guess tinytest is testing the scripts directly without loading the whole stack. meteor-headers works (in the latest versions) by injecting the headers into the initial HTTP request, which is why you won't have them in isolated tests. Is there a way to disable testing for specific packages? Else you're welcome to submit a PR which just fakes this variable on testing (for unit testing only. Obviously you can't create proper tests for packages like this when there is no real client sending real real headers via HTTP).

Nemo64 commented 10 years ago

Fair enough. I'll look into your script as soon as I have time again. As far as I can see your script relies on that __headers__ is defined as soon as it starts. Maybe it would already help if the script wouldn't critically fail in that case. ;)

gadicc commented 10 years ago

I hear you, except it's guaranteed (in a real environment) that __headers__ will always be defined :> It's defined in the HEAD before the script tag which loads headers-client.js (or the entire minified bundle, in production). So there's actually no real situation with a real client that this won't be true, and when not using a real client, well, the concept of HTTP headers doesn't really mean anything. If tested in e.g. laika, this would work (which uses a phantomjs instance as real client).

Nemo64 commented 10 years ago

I just realized that this will probably make this script incompatible with meteors browser-policy package with disallowInlineScripts(). Haven't tested that though and I wouldn't recommend to use that disallow but might be interesting to mention in your docs ;)

gadicc commented 10 years ago

Yes, you're right. I see Meteor has a workaround for this for __meteor_runtime_config__, which works on the same principle. I'll open a seperate issue for this, https://github.com/gadicohen/meteor-headers/issues/19.

gadicc commented 10 years ago

Oh btw, the new code doesn't rely on this being available (for when appcache or browser-policy with disallowInlineScripts() is allowed), does tinytest work now?

Also, thanks for everything you pointed out today... these were all very important issues and I'm glad they're all resolved now! A lot of people are very grateful for this even if they don't realize it :)

Nemo64 commented 10 years ago

I can't check it right now but because i'm at work (and unfortunately don't work with meteor normally ;)). I'll tell you as soon as i can.

Nemo64 commented 10 years ago

Well, the test doesn't fail with a critical error anymore, however the request to headersHelper.js fails but that might be an error of meteors tinytest. I suspect a lot of errors with requests in tinytest because even resources I make public in my package aren't correctly available.

Anyways the tests aren't interrupted anymore which already helps a lot!

gadicc commented 10 years ago

Yeah, I think this will be a problem for tinytest. I guess we probably want laika in here at some point.