Broken in FF, How do you run tests? #12

Open
kirbysayshi opened this Issue Feb 20, 2013 · 17 comments

Comments

Projects
None yet
6 participants
@kirbysayshi

browser-request is broken in FF, due to the following code:

for (key in window) // XMLHttpRequest is not enumerated in FF
    if(window.hasOwnProperty(key))
      win[key] = window[key]

  run_code()

  for (key in window)
    if(window.hasOwnProperty(key))
      if(window[key] !== win[key]) {
        exports[key] = window[key] // ... therefore it's never exported here
        window[key] = win[key]
      }
// therefore the exports object is empty and this throws
var xmlhttprequest = require('./xmlhttprequest')
if(!xmlhttprequest || typeof xmlhttprequest !== 'object')
  throw new Error('Could not find ./xmlhttprequest')

This currently breaks voxel-gist in FF (cc @maxogden).

I'd like to submit a PR, but can't figure out how to run tests. rake test fails because it's trying to hit http://localhost:5984/test-browser-request, which I have no idea how to create.

Also, before attempting a fix, I wanted to ask why the for ... in loop was being used at all, because I believe XMLHttpRequest (the library, not the built-in) only exports XMLHttpRequest, meaning the exports are known. Unless there are others that I don't know about?

@jonpacker

This comment has been minimized.

Show comment Hide comment
@jonpacker

jonpacker Apr 30, 2013

+1, this is causing problems for me also.

+1, this is causing problems for me also.

@maxogden

This comment has been minimized.

Show comment Hide comment
@maxogden

maxogden Jul 5, 2013

Contributor

@jhs if you add me as a collabo to this repo i'll fix junk

Contributor

maxogden commented Jul 5, 2013

@jhs if you add me as a collabo to this repo i'll fix junk

@jhs

This comment has been minimized.

Show comment Hide comment
@jhs

jhs Jul 6, 2013

Member

@maxogden Thanks very much!

I believe you already have access, since you are a member of the iriscouch/colleagues group. Please let me know if that is only read-only or something like that.

Member

jhs commented Jul 6, 2013

@maxogden Thanks very much!

I believe you already have access, since you are a member of the iriscouch/colleagues group. Please let me know if that is only read-only or something like that.

@maxogden

This comment has been minimized.

Show comment Hide comment
@maxogden

maxogden Jul 6, 2013

Contributor

OH!

On Fri, Jul 5, 2013 at 5:04 PM, Jason Smith notifications@github.comwrote:

@maxogden https://github.com/maxogden Thanks very much!

I believe you already have access, since you are a member of the
iriscouch/colleagues group. Please let me know if that is only read-only or
something like that.


Reply to this email directly or view it on GitHubhttps://github.com/iriscouch/browser-request/issues/12#issuecomment-20544448
.

Contributor

maxogden commented Jul 6, 2013

OH!

On Fri, Jul 5, 2013 at 5:04 PM, Jason Smith notifications@github.comwrote:

@maxogden https://github.com/maxogden Thanks very much!

I believe you already have access, since you are a member of the
iriscouch/colleagues group. Please let me know if that is only read-only or
something like that.


Reply to this email directly or view it on GitHubhttps://github.com/iriscouch/browser-request/issues/12#issuecomment-20544448
.

@maxogden maxogden referenced this issue in maxogden/requirebin Jul 6, 2013

Closed

Fails to start on Chromium 28 #1

@maxogden

This comment has been minimized.

Show comment Hide comment
@maxogden

maxogden Jul 7, 2013

Contributor

I fixed this by changing the line in XMLHTTPRequest from:

var XHR = xmlhttprequest.XMLHttpRequest

to

var XHR = xmlhttprequest.XMLHttpRequest || XMLHttpRequest

but there are issues:

  • the submodule for xmlhttprequest in this repo is old and needs to be updated
  • I don't know how to run the tests
  • I don't know how to build a new version of the library

it would be nice if npm test and npm run build were implemented to do these

Contributor

maxogden commented Jul 7, 2013

I fixed this by changing the line in XMLHTTPRequest from:

var XHR = xmlhttprequest.XMLHttpRequest

to

var XHR = xmlhttprequest.XMLHttpRequest || XMLHttpRequest

but there are issues:

  • the submodule for xmlhttprequest in this repo is old and needs to be updated
  • I don't know how to run the tests
  • I don't know how to build a new version of the library

it would be nice if npm test and npm run build were implemented to do these

@jhs

This comment has been minimized.

Show comment Hide comment
@jhs

jhs Jul 7, 2013

Member

I fixed the submodule. Building is now available via npm run build.

The tests require CouchDB running on localhost:5984. Me, I would rather go out in public with no pants on than not have couch at localhost:5984; but perhaps that is not a broadly-held opinion.

@maxogden what is your sense of Ender? Are people using it anymore? Is it worth continued support? It was the dog's bollocks when I wrote this but I don't follow browser JS trends much. Thanks.

Member

jhs commented Jul 7, 2013

I fixed the submodule. Building is now available via npm run build.

The tests require CouchDB running on localhost:5984. Me, I would rather go out in public with no pants on than not have couch at localhost:5984; but perhaps that is not a broadly-held opinion.

@maxogden what is your sense of Ender? Are people using it anymore? Is it worth continued support? It was the dog's bollocks when I wrote this but I don't follow browser JS trends much. Thanks.

@maxogden

This comment has been minimized.

Show comment Hide comment
@maxogden

maxogden Jul 7, 2013

Contributor

im pretty sure nobody uses ender anymore

On Sat, Jul 6, 2013 at 5:59 PM, Jason Smith notifications@github.comwrote:

I fixed the submodule. Building is now available via npm run build.

The tests require CouchDB running on localhost:5984. Me, I would rather go
out in public with no pants on than not have couch at localhost:5984; but
perhaps that is not a broadly-held opinion.

@maxogden https://github.com/maxogden what is your sense of Ender? Are
people using it anymore? Is it worth continued support? It was the dog's
bollocks when I wrote this but I don't follow browser JS trends much.
Thanks.


Reply to this email directly or view it on GitHubhttps://github.com/iriscouch/browser-request/issues/12#issuecomment-20563758
.

Contributor

maxogden commented Jul 7, 2013

im pretty sure nobody uses ender anymore

On Sat, Jul 6, 2013 at 5:59 PM, Jason Smith notifications@github.comwrote:

I fixed the submodule. Building is now available via npm run build.

The tests require CouchDB running on localhost:5984. Me, I would rather go
out in public with no pants on than not have couch at localhost:5984; but
perhaps that is not a broadly-held opinion.

@maxogden https://github.com/maxogden what is your sense of Ender? Are
people using it anymore? Is it worth continued support? It was the dog's
bollocks when I wrote this but I don't follow browser JS trends much.
Thanks.


Reply to this email directly or view it on GitHubhttps://github.com/iriscouch/browser-request/issues/12#issuecomment-20563758
.

@jhs

This comment has been minimized.

Show comment Hide comment
@jhs

jhs Jul 7, 2013

Member

Thanks. Also, if var XHR = xmlhttprequest.XMLHttpRequest fails then that is a bug in my building or packaging. The intention is that xmlhttprequest is a global variable (or that it is correctly in browserify, etc.)

The whole point of using the xmlhttprequest project as a submodule is that it standardizes the API and behavior across all browsers.

Member

jhs commented Jul 7, 2013

Thanks. Also, if var XHR = xmlhttprequest.XMLHttpRequest fails then that is a bug in my building or packaging. The intention is that xmlhttprequest is a global variable (or that it is correctly in browserify, etc.)

The whole point of using the xmlhttprequest project as a submodule is that it standardizes the API and behavior across all browsers.

@kirbysayshi

This comment has been minimized.

Show comment Hide comment
@kirbysayshi

kirbysayshi Jul 7, 2013

Glad to see that progress is being made on this.

Note: I am one of many (I assume) who would rather go into public without pants than require a local couchdb server for a JS library's tests.

Glad to see that progress is being made on this.

Note: I am one of many (I assume) who would rather go into public without pants than require a local couchdb server for a JS library's tests.

@jhs

This comment has been minimized.

Show comment Hide comment
@jhs

jhs Jul 7, 2013

Member

We will have to agree to disagree that CouchDB provides a feeling of warmth, comfort, and security.

I respect your opinion, and I support diversity; so I will work on removing couch as a test dependency. Really, all that is needed is a static file server, which Node itself can do.

Member

jhs commented Jul 7, 2013

We will have to agree to disagree that CouchDB provides a feeling of warmth, comfort, and security.

I respect your opinion, and I support diversity; so I will work on removing couch as a test dependency. Really, all that is needed is a static file server, which Node itself can do.

@kirbysayshi

This comment has been minimized.

Show comment Hide comment
@kirbysayshi

kirbysayshi Jul 7, 2013

That sounds like a good plan, especially if all that is required is to be able to serve static files. There are several modules that will do that (quickserve, serve, paperboy...).

Also, I have nothing against couchdb! Only that an external tech is a impediment to others contributing.

That sounds like a good plan, especially if all that is required is to be able to serve static files. There are several modules that will do that (quickserve, serve, paperboy...).

Also, I have nothing against couchdb! Only that an external tech is a impediment to others contributing.

@jhs jhs referenced this issue in iriscouch/follow Jul 9, 2013

Open

Cannot build for browser #24

@mandric mandric referenced this issue in medic/medic-webapp Jul 10, 2013

Closed

changes feed listener breaks frequently #252

@maxogden maxogden referenced this issue in maxogden/requirebin Jul 10, 2013

Open

Bad XMLHttpRequest in Firefox (Nightly) #20

@maxogden

This comment has been minimized.

Show comment Hide comment
@maxogden

maxogden Jul 10, 2013

Contributor

@jhs i'm pretty sure that something about the way you're loading the xmlhttprequest polyfill is broken because I cant get it to work in firefox without adding || XMLHttpRequest to the end like I said above

Contributor

maxogden commented Jul 10, 2013

@jhs i'm pretty sure that something about the way you're loading the xmlhttprequest polyfill is broken because I cant get it to work in firefox without adding || XMLHttpRequest to the end like I said above

@maxogden

This comment has been minimized.

Show comment Hide comment
@maxogden

maxogden Jul 14, 2013

Contributor

ok I have a pretty radical fork https://github.com/maxogden/browser-request

on the plus side it has testling tests

Contributor

maxogden commented Jul 14, 2013

ok I have a pretty radical fork https://github.com/maxogden/browser-request

on the plus side it has testling tests

@maxogden

This comment has been minimized.

Show comment Hide comment
@maxogden

maxogden Jul 18, 2013

Contributor

@jhs do you approve of my house cleaning here?

Contributor

maxogden commented Jul 18, 2013

@jhs do you approve of my house cleaning here?

@indexzero

This comment has been minimized.

Show comment Hide comment
@indexzero

indexzero Aug 5, 2013

Owner

+1 I'm not a browser expert these days, but "browserify ftw" seems like a good approach!

Owner

indexzero commented Aug 5, 2013

+1 I'm not a browser expert these days, but "browserify ftw" seems like a good approach!

@maxogden

This comment has been minimized.

Show comment Hide comment
@maxogden

maxogden Aug 5, 2013

Contributor

made a pull req #17

Contributor

maxogden commented Aug 5, 2013

made a pull req #17

@paulmelnikow

This comment has been minimized.

Show comment Hide comment
@paulmelnikow

paulmelnikow Jan 14, 2015

This still seems to be a problem in the latest version from npm (0.2.1). Works in Chrome, but not in Firefox.

This still seems to be a problem in the latest version from npm (0.2.1). Works in Chrome, but not in Firefox.

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