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

Check if window.fetch exists before polyfilling it #362

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bobziroll
Copy link

Sorry if this is presumptuous of me, I haven't done much by way of contributing code to OS before, so please let me know if I need to do something before submitting this.

I'm running into an issue where whatwg-fetch is hijacking the Response object that React Router checks when redirecting. React Router ≥v6.4.5 duck-type checks the Response object, checking specifically if there is a body property included. (Since it's required by W3 Spec).

I guess whatwg-fetch doesn't support the body property, but instead injects a _bodyInit property in its place. Something to do with ReadableStreams or whatnot, I'm not sure 😝

That said, I was curious why pretender is using the polyfill in the first place, since my browser is a modern, fetch-enabled browser.

Please let me know if there's another, better way I could contribute to this, or help me better understand why a PR like this doesn't make sense. Thanks in advance! 🙏

@vincicat
Copy link

vincicat commented Aug 7, 2023

Please accept this PR as it will solve the Pretender Error in node.js 18+

@vincicat
Copy link

vincicat commented Aug 7, 2023

@bobziroll Thank you for your help.

I got error in other line that iterating this._fetchProps (e.g. dist/pretender.js:1809:27), may be this._fetchProps need to be an empty array by default to ensure no more code will be broken...

@fekete965
Copy link

@bobziroll Thank you for your help.

I got error in other line that iterating this._fetchProps (e.g. dist/pretender.js:1809:27), may be this._fetchProps need to be an empty array by default to ensure no more code will be broken...

Yes indeed, at the top of the constructor we need to initialize it to an empty array.

I would also propose to validate the incoming verbs:

const Verbs: Record<string, Verb> = {
  GET: 'GET',
  POST: 'POST',
  PUT: 'PUT',
  DELETE: 'DELETE',
  PATCH: 'PATCH',
  HEAD: 'HEAD',
  OPTIONS: 'OPTIONS',
} as const;

const validatedVerb = (str: string): Verb => {
  str = str.toUpperCase();

  if (!Verbs[str]) {
    const validVerbs = Object.values(Verbs).join(', ');
    throw new Error(
      `${str} is not a valid verb. Expected value one of [${validVerbs}]`
    );
  }

  return Verbs[str];
};

I also believe the map on the Pretender instance itself is wrongly typed:

map(fn: (pretender: Pretender) => void) {
  n.call(this);
}

I believe this should be:

map(fn: (this: Pretender) => void) {
  fn.call(this);
}

This way we tie what context the function can be called with.

A better API would be:

map(fn: (this: Pretender) => void): Pretender {
  fn.call(this);
  
  return this;
}

This way we could nicely chain our operations together:

const myPretender = new Pretender()
  .map(authenticationRoutes);
  .map(songsRoutes);

Now I can also see another Type error in the following line:

requiresManualResolution(verbStr: string, path: string) {
  let verb = validatedVerb(verbStr);
  let handler = this._handlerFor(verb, path, {}); <-- object is not a valid argument
  // etc...
}

this._handlerFor accepts a FakeRequest but an empty object is not valid.
I am not sure that this is intended or that we should have a factory that creates fake requests for us.

Sorry @bobziroll if I spammed your PR here. 😅

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.

3 participants