search

grantila / fetch-h2

HTTP/1+2 Fetch API client for Node.js
MIT License
336 stars 16 forks source link

convert Headers to extend Map #69

Closed colinbendell closed 4 years ago

colinbendell commented 4 years ago

the Headers object should extend Map() so that standard mocha/chai specs treat this as a Map and utilise the standard methods

coveralls commented 4 years ago

Coverage Status

Coverage increased (+0.2%) to 83.724% when pulling f27002c9d547574e79f155d92bde696edf786fe6 on colinbendell:master into b94058439166afa2f5b716e7c8d0a7b7497eb247 on grantila:master.

colinbendell commented 4 years ago

@grantila what's the process for review and get the patch landed?

grantila commented 4 years ago

I would say the SAN and Headers changes should be separate, but I'm not sure what the actual problem is with the Headers as they are. They follow exactly https://developer.mozilla.org/en-US/docs/Web/API/Headers/Headers so that browser code running in Node.js can expect the very same behavior.

grantila commented 4 years ago

I made Headers look like Map to unit test frameworks (at least this works in Jest) in a less intrusive way.

colinbendell commented 4 years ago

Sorry, the san issue wasn't supposed to merge into this PR.

colinbendell commented 4 years ago

The issue with Headers is that you're doing a lot of work to mimic Map functionality I would argue that treating it as a Map<string, string> is a simpler design and has better language consistency leading to better developer ergonomics for those adopting this library. The only reason that fetch doesn't explicitly define Headers as extending Map is because of the age of the spec.

grantila commented 4 years ago

:tada: This issue has been resolved in version 2.3.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket: