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

Url parse issue #111

Merged
merged 3 commits into from
Jun 10, 2014
Merged

Url parse issue #111

merged 3 commits into from
Jun 10, 2014

Conversation

AdamMagaluk
Copy link
Contributor

Fixes issues in #108, needs Kevin's PR that fixes string.split regex issue #109.

@kevinswiber
Copy link
Contributor

It looks like there may also be a bug in http.js once/if these changes are merged.

exports.get appends the querystring to path, but path should already have the querystring.

Offending block: https://github.com/tessel/runtime/blob/master/src/colony/modules/http.js#L373-L375

Patch:

@@ -370,9 +370,6 @@ exports.request = function (opts, onresponse) {
 exports.get = function (opts, onresponse) {
   if (typeof opts == 'string') {
     opts = url.parse(opts);
-    if (opts.search) {
-      opts.path += opts.search;
-    }
   }
   opts.method = 'GET';

@tcr
Copy link
Member

tcr commented Jun 10, 2014

Going to close and restart this to trigger CI.

@tcr tcr closed this Jun 10, 2014
@tcr tcr reopened this Jun 10, 2014
tcr added a commit that referenced this pull request Jun 10, 2014
@tcr tcr merged commit 0190918 into tessel:master Jun 10, 2014
@tcr
Copy link
Member

tcr commented Jun 10, 2014

Thanks for this! We should be moving toward reusing as much Node code as possible, rather than our old demo code.

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.

None yet

3 participants