Skip to content

Conversation

@DarkGL
Copy link
Contributor

@DarkGL DarkGL commented Jun 20, 2024

This relates to...

#2552

Rationale

This is version of dns cache with resolved merge conflicts and tests migrated to node:test, most of the work is done by @antoinegomez

Changes

Features

Bug Fixes

Breaking Changes and Deprecations

Status

@DarkGL
Copy link
Contributor Author

DarkGL commented Jun 20, 2024

I need help with fixing test on mac os (works on windows and linux)

Test:

test('http example', async (t) => {
  t = tspl(t, { plan: 1 })

  const cacheable = new DNSResolver({ resolver })

  const options = {
    hostname: 'example',
    port: 9999,
    lookup: cacheable.lookup
  }

  await t.rejects(makeRequest(options), {
    message: 'connect ECONNREFUSED 127.0.0.127:9999'
  })
})

const makeRequest = (options) =>
  new Promise((resolve, reject) => {
    http.get(options, resolve).once('error', reject)
  })

on mac os this test fails with:

✖ test/dns-resolver.js (30015.227ms)
  'test timed out after 30000ms'

on windows and linux this test resolves correctly

Copy link
Member

@ronag ronag left a comment

Choose a reason for hiding this comment

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

I don't think this is necessary. Already possible:

import CacheableLookup from 'cacheable-lookup';

const cacheable = new CacheableLookup(opts)

const agent = new Agent({
  connect: { lookup: cacheable.lookup }
})

@DarkGL
Copy link
Contributor Author

DarkGL commented Jun 20, 2024

Mentioning reviewers from orginal pr @mcollina @ShogunPanda @metcoder95 @KhafraDev , should I continue or close this pr ?

@KhafraDev
Copy link
Member

As ronag mentioned it's already possible to use cacheable-lookup

@KhafraDev
Copy link
Member

The only reason I could see for adding it is if we wanted to enable it by default, but we're probably better off just documenting it

@DarkGL
Copy link
Contributor Author

DarkGL commented Jun 21, 2024

@ronag
Copy link
Member

ronag commented Jun 21, 2024

Examples

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.

4 participants