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

Add custom request API #174

Merged
merged 3 commits into from
Oct 12, 2019

Conversation

ubarsaiyan
Copy link
Contributor

Closes #169
This PR also includes #168 by @yoh1496 as it is instrumental in sending custom requests that contain a body.
I have included one test that sends a stat request and passes the stat checks.

Copy link
Owner

@perry-mitchell perry-mitchell left a comment

Choose a reason for hiding this comment

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

1 item to change. Also the tests are failing, due to object spreading.. You can use Object.assign instead if that helps.

source/interface/stat.js Show resolved Hide resolved
@ubarsaiyan
Copy link
Contributor Author

ubarsaiyan commented Aug 23, 2019

@perry-mitchell sorry but I don't get where have I used object spreading? Can you please point to it. I'll then fix asap so it can be merged. I looked at the logs and I don't think I have changed any of the files mentioned in the logs.

@perry-mitchell
Copy link
Owner

@ubarsaiyan You’re right, sorry, it seems that it’s a dependency - hoek. No idea why it’s failing now but I can’t merge this until the tests pass.

@intel352
Copy link

intel352 commented Oct 3, 2019

@perry-mitchell So the last fully passing tests were 3 months ago. 2 months ago, this PR branch and your platform-upgrade branch both started failing on Node 6 and WEB.

It might be worth forcing tests to run against master right now, to make sure it's still fully passing. Perhaps one of your dependencies is no longer compatible with Node 6, so you perhaps need to add a restriction on the dependency version.

That being said, while I'm sure some people use the browser support, how important is it to continue servicing Node 6? Node 6 stopped being supported in May 2019.
Node 8 will stop being supported in Jan 2020.

I'd suggest you at least drop Node 6.

@intel352
Copy link

intel352 commented Oct 3, 2019

@ubarsaiyan Thanks for this branch. Testing this, I would expect customRequest to behave the same as the other requests, in that the base URL should be prepended to a partial URL path.
If you want to support wholly custom URL endpoints in the customRequest, then I'd suggest you should additionally provide a static method for customRequest.

Otherwise, I don't see what benefit is gained from using customRequest on the client instance with the same auth/credentials but the potential to post to a wholly different domain.

Copy link

@intel352 intel352 left a comment

Choose a reason for hiding this comment

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

@ubarsaiyan something like this might work

source/interface/custom.js Outdated Show resolved Hide resolved
source/factory.js Outdated Show resolved Hide resolved
source/factory.js Outdated Show resolved Hide resolved
test/specs/customRequest.spec.js Outdated Show resolved Hide resolved
@ubarsaiyan
Copy link
Contributor Author

ubarsaiyan commented Oct 9, 2019

@intel352 Thank you for your suggestions. I have made the required changes. cc: @perry-mitchell @ggazzo

@perry-mitchell
Copy link
Owner

Great, so all that's left is to solve the failing tests.. Node 6:

/home/travis/build/perry-mitchell/webdav-client/node_modules/@hapi/joi/lib/types/object/index.js:255
                        !pattern.schema._validate(key, state, { ...options, abortEarly:true }).errors) {
                                                                ^^^
SyntaxError: Unexpected token ...

And web:

$ export DISPLAY=:99.0
0.00s$ sh -e /etc/init.d/xvfb start
sh: 0: Can't open /etc/init.d/xvfb
The command "sh -e /etc/init.d/xvfb start" failed and exited with 127 during .

Looking into this on master now..

@ubarsaiyan
Copy link
Contributor Author

@perry-mitchell yes I think this is not specific to my PR. Thanks!

@perry-mitchell
Copy link
Owner

Node issue related to: jeffbski/wait-on#44

@perry-mitchell
Copy link
Owner

@ubarsaiyan Master should be fixed.. could you please merge?

@ubarsaiyan
Copy link
Contributor Author

@perry-mitchell done

@perry-mitchell perry-mitchell merged commit 39c39f4 into perry-mitchell:master Oct 12, 2019
@perry-mitchell
Copy link
Owner

I won’t be home for a bit, but I’ll most likely release later today. Thanks!

@perry-mitchell
Copy link
Owner

Released under 2.10.0! Thanks

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.

Custom request API
3 participants