-
Notifications
You must be signed in to change notification settings - Fork 141
Conversation
Just got back from a short vacation and so many things happened here O.o I'm gonna review all the changes asap |
@codemanki You might want to hold off until I push these tests |
The code coverage is sane again but not quite 100% |
Hey, I've changed my mind. I'm not going to add any extra additional tests for the time being but the PR does LGTM. I can address any issues if you see any. |
Also to clarify, Brotli is a completely optional dependency but if anybody intends on having bulletproof headers, they should install it. |
@VeNoMouS you might want to review that last comment I made. |
One last thing to mention is that there is some percentage of the user base that is facing issues that this PR resolves by beating Cloudflare's updated BIC checks. |
There are no breaking changes, right? |
No breaking changes other than what I noted above... |
TBH, it's sitting here when it works better than the master branch. I consider the current release to be broken. |
I will release it today
…On Sun, Apr 21, 2019, 11:58 Dwayne ***@***.***> wrote:
TBH, it's sitting here when it works better than the master branch. I
consider the current release to be broken.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#198 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AACK6QEYBAREW6S4A5F4IL3PRQ3FZANCNFSM4HF5UTBA>
.
|
@codemanki Before you do, check the |
@pro-src could you please change the target branch from master to https://github.com/codemanki/cloudscraper/tree/4.0.0 ? |
Done. |
} | ||
|
||
stringBody = body.toString('utf8'); | ||
|
||
try { | ||
validate(options, response, stringBody); | ||
} catch (error) { | ||
if (error instanceof CaptchaError && typeof options.onCaptcha === 'function') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to document this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would add a few examples as a part of that documentation. See the relevant issue for examples.
@pro-src i will then merge this PR, will add my changes to the branch and will create a new PR. Otherwise I can't add my changes to your branch |
@pro-src Oh i see. It is indeed possible - https://help.github.com/en/articles/committing-changes-to-a-pull-request-branch-created-from-a-fork |
AFAIK you should be able to change the target branch back to master should you need to and change the title, etc. etc. 😉 |
Yes, it is safe. 💣 |
I guess it is ready to be released. @pro-src do you want to take a quick look? |
Travis tests have failedHey @pro-src, Node.js: nodenpm test
Node.js: 6npm test
TravisBuddy Request Identifier: b9d627a0-6511-11e9-a049-ff6f6c17598f |
Probably should make a dummy I usually do the following before committing: LGTM 👍 |
Here we go... Thank you @pro-src for this PR :) |
Supersedes #196
lib/browsers.json
- contains UA and exact header informationlib/headers.js
- loads a random UA fromlib/browsers.json
var
with eitherconst
orlet
document.getElementById
lib/brotli.js
for the optional support (node's or user installed version)request
andbrotli
peer dependencies which must be installed by the user.lib/sandbox.js
and increases testabilityprocesssResponseBody
->onRequestComplete
validateRequest
If a user attempts to run cloudscraper without first installing
request
, they will receive the following error message:The error message is automatically produced by
request-promise
.Closes #147
Closes #62