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

feat(scully): allow specify headers when using the JSON plugin #140

Merged

Conversation

d-koppenhagen
Copy link
Contributor

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Other... Please describe:

What is the current behavior?

Issue Number: #137

What is the new behavior?

Users can specify a headers object in ths scully.config.js.
This headers will be passed throught the request when calling the specified endpoint.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

I added the request module instead of using http and https and distinguish between this ones as it will abstract this already and allows to specify the options like headers without the need to pass a specific options object and splitting the URL manually into host, path and so on.

user can specify a `headers` object in ths `scully.config.js`.
This headers will be passed throught the request when calling the
specified endpoint.

Closes scullyio#137
@d-koppenhagen
Copy link
Contributor Author

Unfortunatly I couln't find any test specs which I could extend. Also I haven't had the chance to test this against a real API which needs a specific header.
Are you able to test this implementation a bit better or to guide me how to do it?

@d-koppenhagen d-koppenhagen changed the title WIP: feat(scully): allow specify headers when using the JSON plugin feat(scully): allow specify headers when using the JSON plugin Jan 4, 2020
rawData.map(row => deepGet(conf[param.part].property, row))
);
return httpGetJson(url, {
headers: conf[param.part].headers,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this error if there aren't any provided? Does it work without anything?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm good question I currently don't know how to test it locally ^^ -- probably I need to add || {} to it in case it's null or undefined. An empty object should be fine I think.

Copy link
Contributor

@SanderElias SanderElias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@aaronfrost aaronfrost merged commit 8415355 into scullyio:master Jan 4, 2020
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.

3 participants