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

release: 2.0.0 #162

Merged
merged 22 commits into from
Oct 12, 2021
Merged

release: 2.0.0 #162

merged 22 commits into from
Oct 12, 2021

Conversation

szmarczak
Copy link
Contributor

@szmarczak szmarczak commented Oct 6, 2021

done

wip: simplify forward

  • remove babel
  • simplify forward handler
  • added https support
    • test
  • require node.js 14
  • skipped 2 tests (will re-enable once proxy-chain is fully rewritten)

wip: simplify custom response

  • simplify custom response
  • use shared eslint config

wip: simplify direct

  • simplify direct

wip: simplify chain

  • simplify chain
    • remove handler base

wip: simplify tools

  • simplify tools
    • remove parseUrl(...) in favor of new URL(...)
    • remove parseHostHeader(...) in favor of new URL(...)
    • remove isInvalidHeader(...) (unused)
    • tests: remove buggy findFreePort in favor of port: 0
    • simplify redactUrl
    • remove addHeader (leftover from wip: simplify chain #156)
  • add IPv6 tests

There's a slight behavior change in parseAuthorizationHeader. It now returns { type: '', data: '' } (instead of null) if the proxy-authorization header is invalid.

wip: simplify tunnel

wip: simplify server

other

wip: move to TypeScript

  • use typescript
  • manual testing

@szmarczak
Copy link
Contributor Author

szmarczak commented Oct 6, 2021

I have tested this manually and found no issues. Chain, Custom Response, Direct, Forward - they all work.

IPv6 works as well (if upstream supports it, not all proxies do).

@szmarczak szmarczak requested review from jancurn, mnmkng and B4nan October 6, 2021 17:13
@temp3l
Copy link

temp3l commented Oct 7, 2021

awesome module!
currently seeing some errors in example/apify_proxy_tunnel.js (@next)

@szmarczak
Copy link
Contributor Author

What errors are you seeing?

Copy link
Member

@mnmkng mnmkng left a comment

Choose a reason for hiding this comment

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

I'm pretty sure I won't find any issues now as all the PRs were reviewed individually. Let's wait for @jancurn's approval and then proceed with limited testing.

Copy link
Member

@jancurn jancurn left a comment

Choose a reason for hiding this comment

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

I did a review of the new code. The code feels much much cleaner, it's broken down to more files, everything in TypeScript... in summary, definitely a big improvement, great work @szmarczak ! I didn't go into the lower-level details, I guess we'll have to gradually test it under heavier and heavier loads and see how it works.

Here are some comments:

  • Altogether, the code is commented only very sparingly. A lot of functions don't have any comments, so you need to analyze the code to understand what they do. Sometimes their name doesn't say much too (e.g. getBasic, redactUrl). When the functions are internal and are a few lines long, then it's fine, but sometimes they are longer or serve an important case, and then they should be commented. At the very least, all public methods of Server class should be commented, as well as core functions like chain, handleCustomResponse, direct, forward, ...
  • Please can you also update the CHANGELOG?
  • It would be great if we could generate documentation of all the public classes and functions, now we have just README. Maybe in the next version...
  • Are we using all the unit tests as we did in the past? I remember some things were commented out

@szmarczak
Copy link
Contributor Author

Yep, all the tests pass. The commented out tests are now uncommented and pass as well :)

Copy link
Member

@B4nan B4nan left a comment

Choose a reason for hiding this comment

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

Agreed regarding the comments, while I understand the motivation behind removing comments that are not helpful, it does not mean we should have no comments :] At least public API should be documented.

It's always good to try to look at the comments from a newbie point of view, while it might feel moot for you, there still can be some value for newbies/newcomers.

Btw what about #160, is that addressed by the PR too?
I see it is, so the PR desc should contain the closes/fixes part to link it (or is it enough like this, just fix?).

@szmarczak
Copy link
Contributor Author

szmarczak commented Oct 12, 2021

Altogether, the code is commented only very sparingly.

I'm trying to write self explanatory code when possible.

  • countTargetBytes does exactly what it says, it starts to count read and written bytes of the target socket.
  • decodeURIComponentSafe, safe means it doesn't throw. This is a common practice.
  • I agree that getBasic doesn't say much, I've renamed it to getBasicAuthorizationHeader.
  • isHopByHopHeader is self explanatory.
  • parseAuthorizationHeader is self explanatory as well, no need to explicitly view the code since we have the TypeScript definitions.
  • redactUrl may be not so self explanatory, but again - the parameters are url and passwordReplacement, and that is self explanatory :)
  • Renamed the array parameter in validHeadersOnly to rawHeaders, also linked to Node.js docs. Should be enough :)

At the very least, all public methods of Server class should be commented

They already are, not sure what you mean. Those comments just need corrections, will fix them.

as well as core functions like chain, handleCustomResponse, direct, forward, ...

Done 👍🏼

Please can you also update the CHANGELOG?

On it.

It would be great if we could generate documentation of all the public classes and functions, now we have just README.

Agreed. However generators are not so friendly. Most projects do docs by hand.

@B4nan

Btw what about #160, is that addressed by the PR too?
I see it is, so the PR desc should contain the closes/fixes part to link it (or is it enough like this, just fix?).

image

image

Already there ;)

@szmarczak szmarczak merged commit f1bbe42 into master Oct 12, 2021
@szmarczak szmarczak deleted the next branch October 12, 2021 07:24
@szmarczak szmarczak mentioned this pull request Oct 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants