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

full support for node url object #28

Closed
wants to merge 2 commits into from

Conversation

panthershark
Copy link

Fixed this issue - #27

Attached uri to the Request instance so that I could write a few unit tests. Added unit tests for simple case and for full node url object.

Please let me know if there is anything further I can do.

@ameensol
Copy link

@substack Would be awesome if you could implement this pull request. The node-alchemy module is currently incompatible with browserify due to the lack of support for node url object. See framingeinstein/node-alchemy#4.

@panthershark
Copy link
Author

@ameensol - Here is my current workaround until this PR is merged.

in package.json put this entry.
"http-browserify": "git+https://github.com/tommydudebreaux/http-browserify.git"

then in the browserify settings, create an alias that points from "http" to "./node_modules/http-browserify/index.js"

@ghost
Copy link

ghost commented Oct 14, 2013

Please don't check in gitignore files ever. They cause conflicts if a gitignore already exists.

@ameensol
Copy link

@tommydudebreaux I already built my own workaround by forking the node-alchemy module and using browser-request instead of http, but thanks anyways!

@panthershark
Copy link
Author

bump

ghost pushed a commit that referenced this pull request Dec 1, 2013
@ghost
Copy link

ghost commented Dec 1, 2013

Merged in 0.3.0. Make sure to bug https://github.com/alexgorbatchev/node-browser-builtins to get this into browserify core.

@ghost ghost closed this Dec 1, 2013
@ghost
Copy link

ghost commented Dec 2, 2013

ping @aredriel we should make sure this patch didn't regress over the pull/30 patch that went in. I had to merge this by hand since it touched a lot of the same lines and there was some movement of the scheme handling too.

@panthershark
Copy link
Author

@substack - Thanks for the merge.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants