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

reproduce mergeHeaders typeerror #268

Conversation

belgattitude
Copy link

@belgattitude belgattitude commented Jul 3, 2020

DO NOT MERGE, just a test, just downtracking

Related to #264.

  • Reproduce type error failure on master branch -> 4cbc981 (breaks as expected)
  • Testing a quick fix -> 7def7f3 (pass as expected)

@belgattitude
Copy link
Author

belgattitude commented Jul 3, 2020

@sholladay trying to fix the #264 in the constructor rather than in the mergeHeaders().

So here's the reproduction case when doing typecheck in the mergeHeaders (most browsers will assume undefined as {}, but this reproduce the case, looks a legit to me).

Fix is not not a problem.

Test for me is hard to do. Not used to ava, no constructor exposed. I appreciate your guidance

What I try to achieve:

  • constructor
    • Given that inputis a string (ie: '/api/test')
      • the headers are taken from argument options.headers
    • Given than input is an URL or global.Request (and have headers defined).
      • the headers must be merged with arguments in options.headers

There's no reason why we would want to throw a type error when input is a string (it's part of the contract)

An exhaustive version of the constructor would reflect the idea +/- like this

class Ky {
  /**
   * @param {string|URL|globals.Request|null|undefined} input
   * @private
   */
  constructor(input, options = {}) {
   let headers;
   if (isObject(input) && isObject(input.headers)) {
     headers = mergeHeaders(input.headers,  isObject(options.headers) ? options.headers : {};);
   } else {
     headers = isObject(options.headers) ? options.headers : {};
   }
   /// here a lot more
   this._options = {
     headers
   }

awful ;)

The alternative approach still exists in my mind: setting default {} in mergeHeaders when the type is wrong.

I know modern browsers is what targets ky and this should not be needed.

But unluckiy this is a requirement here. A lot of errors in sentry for us

@@ -49,6 +49,10 @@ const supportsStreams = typeof globals.ReadableStream === 'function';
const supportsFormData = typeof globals.FormData === 'function';

const mergeHeaders = (source1, source2) => {
if (!isObject(source1)) {
Copy link
Author

Choose a reason for hiding this comment

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

cause yes mergeHeaders will fail in edge 18 (maybe others too)...

@@ -224,11 +228,12 @@ class Ky {
constructor(input, options = {}) {
this._retryCount = 0;
this._input = input;
const headers = isObject(this._input.headers) ? this._input.headers : {};
Copy link
Author

Choose a reason for hiding this comment

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

This can be simplified ! just checking what's the practice here.

Possible

  this._options = {
    headers: mergeHeaders(this._input.headers || {}, options.headers);
  }

@belgattitude
Copy link
Author

Closing as just it was included in #264

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.

1 participant