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

fix: unify connect options #881

Merged
merged 2 commits into from
Jul 16, 2021
Merged

fix: unify connect options #881

merged 2 commits into from
Jul 16, 2021

Conversation

ronag
Copy link
Member

@ronag ronag commented Jul 15, 2021

Should be fully backwards compatible.

@ronag
Copy link
Member Author

ronag commented Jul 15, 2021

I had to remove a test as it incorrectly relies on mutating options. Also not sure how that could work at all as the session cache does not key on cipher.

@ronag ronag requested a review from mcollina July 15, 2021 13:02
@ronag ronag mentioned this pull request Jul 16, 2021
@codecov-commenter
Copy link

codecov-commenter commented Jul 16, 2021

Codecov Report

Merging #881 (595a609) into main (494201a) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##              main      #881   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           26        26           
  Lines         2104      2106    +2     
=========================================
+ Hits          2104      2106    +2     
Impacted Files Coverage Δ
lib/client.js 100.00% <100.00%> (ø)
lib/core/connect.js 100.00% <100.00%> (ø)
lib/pool.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 494201a...595a609. Read the comment docs.

@ronag
Copy link
Member Author

ronag commented Jul 16, 2021

@szmarczak This would allow you to forward a custom lookup function directly to net.connect and tls.connect.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@ronag ronag merged commit 0fccc0c into main Jul 16, 2021
@dnlup
Copy link
Contributor

dnlup commented Jul 16, 2021

I am late to this, sorry. Great work!

@NN-Binary
Copy link

NN-Binary commented Jul 18, 2021

Great job! Any manual on how to change the options (I need to use a custom lookup function) ?

@ronag
Copy link
Member Author

ronag commented Jul 19, 2021

Great job! Any manual on how to change the options (I need to use a custom lookup function) ?

The connect option will be passed into net.connect so you can just do connect: { lookup }.

@NN-Binary
Copy link

I'm not sure how to use it in Dispatch, I've just updated to the latest Undici, how do I add lookup in Dispatch?
Thank you

@ronag
Copy link
Member Author

ronag commented Jul 19, 2021

You don't need to use dispatch. Just pass connect option to Agent constructor. Don't forget you need to run off master since we haven't made a release yet.

@NN-Binary
Copy link

Wow, thank you so much for the quick reply.
Right now my code is quite simple:


                var HTTPClient = new Client('http://google.com')
                HTTPClient.dispatch({
                    path: '/',
                    method: 'GET',
                    headers: {
                        // Headers
                    },
                    lookup: CUSTOM_FUNCTION()
                }, {
                    ...
                })

Agent should be placed in the new Client? Sorry

@ronag
Copy link
Member Author

ronag commented Jul 19, 2021

const client = new Client('http://google.com', {
  connect: { lookup: CUSTOM_FUNCTION }
})

const { body, statusCode, headers } = await client.request({
                    path: '/',
                    method: 'GET',
                    headers: {
                        // Headers
                    },
})

@NN-Binary
Copy link

Oh! You are amazing. thank you so much @ronag

@NN-Binary
Copy link

NN-Binary commented Jul 19, 2021

EDIT: When getting the Master branch, it seems that "Client" doesn't exist anymore.

import { request, Client  } from 'undici';
                  ^^^^^^
SyntaxError: Named export 'Client' not found. The requested module 'undici' is a CommonJS module, which may not support all module.exports as named exports.

npm install git://github.com/nodejs/undici/tree/master --save
No problem here, it fetches Undici but Client Method doesn't exist.

This is the right branch? correct?
Did anything changed between the current version and Master? In term of how I should import it?

From what I'm seeing in index.js, a lot of things changed especially in what is exported or not, is it normal?

Edit: it works if I change my imports to:
import undici from 'undici';

Is it normal that the behavior is different or it's because I don't do it properly?

Well it seems to not work at all anymore, even changing new Client by new undici.Client, I keep getting weird errors
6: 0xd61c66 v8::internal::Builtin_HandleApiCall(int, unsigned long*, v8::internal::Isolate*) [/usr/bin/node]

Sorry I'm lost

@RuralDependencies
Copy link

RuralDependencies commented Jul 19, 2021

EDIT: When getting the Master branch, it seems that "Client" doesn't exist anymore.

import { request, Client  } from 'undici';
                  ^^^^^^
SyntaxError: Named export 'Client' not found. The requested module 'undici' is a CommonJS module, which may not support all module.exports as named exports.

npm install git://github.com/nodejs/undici/tree/master --save
No problem here, it fetches Undici but Client Method doesn't exist.

This is the right branch? correct?
Did anything changed between the current version and Master? In term of how I should import it?

From what I'm seeing in index.js, a lot of things changed especially in what is exported or not, is it normal?

Edit: it works if I change my imports to:
import undici from 'undici';

Is it normal that the behavior is different or it's because I don't do it properly?

Well it seems to not work at all anymore, even changing new Client by new undici.Client, I keep getting weird errors
6: 0xd61c66 v8::internal::Builtin_HandleApiCall(int, unsigned long*, v8::internal::Isolate*) [/usr/bin/node]

Sorry I'm lost

The branch that works with the option you are looking for is here:
https://github.com/nodejs/undici/tree/refactor-write

Master seems behind (version 3)

@ronag
Copy link
Member Author

ronag commented Jul 19, 2021

Please open a separate issue.

@Uzlopak Uzlopak deleted the connect-opts branch February 21, 2024 12:38
crysmags pushed a commit to crysmags/undici that referenced this pull request Feb 27, 2024
* fix: unify connect options

Fixes: nodejs#852

* fixup
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.

None yet

7 participants