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

stream option is lost in any get request that is streamable #188

Closed
yonguelink opened this issue Sep 14, 2018 · 3 comments · Fixed by #195
Closed

stream option is lost in any get request that is streamable #188

yonguelink opened this issue Sep 14, 2018 · 3 comments · Fixed by #195
Milestone

Comments

@yonguelink
Copy link

We detected the issue when using v3.11.4, but I could reproduce it on the latest release (v4.0.1).

We're simply calling downloadSingleArtifactFile as we implemented it (few months back) with the project ID, the job Id, the artifact to download and the options { stream: true}, but we do not receive a stream. The result is the content of the artifact we wanted to download.

After tracing a little, I noticed wrong/missing parameters:

In RequestHelper.get it sends a generic request:

static get(service, endpoint, options = {}, { stream = false } = {}) {
  return RequestHelper.request('get', service, endpoint, options, stream);
}

The RequestHelper.request method is defined as:

static request(type, service, endpoint, options = {}, form = false, stream = false) { ... }

Leading to a call like this:

RequestHelper.request(type = 'get', service = service, endpoint = endpoint, options = options, form = stream, stream = undefined);

Notice how the stream option is placed in the form variable and the stream variable is just never used in this case.

It seems like a simple fix, to call the RequestHelper.request method with something such as this instead:

static get(service, endpoint, options = {}, { stream = false } = {}) {
  return RequestHelper.request('get', service, endpoint, options, !stream, stream);
}

Or take the form definition in the options upstream?

Anyway, it's sure that right now it does not work.

Thank you

@jdalrymple
Copy link
Owner

Thank you for such an in depth investigation! I can try an add that fix this weekend and see if that solves the problem!

@yonguelink
Copy link
Author

yonguelink commented Sep 14, 2018

I did test locally (by modifying the compiled version) and it worked like a charm. It should do the trick!

Thank you for the quick answer!

@jdalrymple jdalrymple added this to the 4.1 milestone Sep 17, 2018
jdalrymple added a commit that referenced this issue Sep 18, 2018
jdalrymple added a commit that referenced this issue Sep 18, 2018
jdalrymple added a commit that referenced this issue Sep 18, 2018
* chore(package): Update jest-extended to the latest version 🚀 0.9.0 (#191)
* chore(package): update lockfile package-lock.json
* feat: Re-add list all project members endpoint (#190, #141)
* feat: Adding markdown support #182 (#193)
* fixes: #188: `stream` option is lost in any `get` request that is streamable (#192)
* feat: Added user snippets API support (#189)
* chore(test): Added test for creating snippets
* feat: Added user edit support #186
@jdalrymple jdalrymple mentioned this issue Sep 18, 2018
Merged
jdalrymple added a commit that referenced this issue Sep 18, 2018
* chore(package): Update jest-extended to the latest version 🚀 0.9.0 (#191)
* chore(package): update lockfile package-lock.json
* feat: Re-add list all project members endpoint (#190, #141)
* feat: Adding markdown support #182 (#193)
* fixes: #188: `stream` option is lost in any `get` request that is streamable (#192)
* feat: Added user snippets API support (#189)
* chore(test): Added test for creating snippets
* feat: Added user edit support #186
@jdalrymple
Copy link
Owner

🎉 This issue has been resolved in version 4.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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 a pull request may close this issue.

2 participants