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

Referrer and Referrer Policy #1057

Merged
merged 4 commits into from Nov 5, 2021
Merged

Referrer and Referrer Policy #1057

merged 4 commits into from Nov 5, 2021

Conversation

tekwiz
Copy link
Member

@tekwiz tekwiz commented Jan 3, 2021

What is the purpose of this pull request?

  • Documentation update
  • Bug fix
  • New feature
  • Other, please explain:

What changes did you make? (provide an overview)

This adds support for the referrer and referrerPolicy options.

Which issue (if any) does this pull request address?

#1036 #839

Is there anything you'd like reviewers to know?

Specifications are referenced in-line.

@tekwiz tekwiz requested a review from jimmywarting Jan 3, 2021
@tekwiz tekwiz linked an issue Jan 3, 2021 that may be closed by this pull request
Copy link
Member

@tinovyatkin tinovyatkin left a comment

Shouldn't TypeScript types also be updated for these changes?

@tekwiz
Copy link
Member Author

@tekwiz tekwiz commented Jan 17, 2021

@tinovyatkin You are quite right. I've updated the TS types

@tekwiz tekwiz requested review from bitinn and removed request for jimmywarting Feb 28, 2021
@YakovL
Copy link

@YakovL YakovL commented Jun 30, 2021

Hi, is it just lack of reviews that stops this from getting merged?

@YakovL
Copy link

@YakovL YakovL commented Jul 1, 2021

@jimmywarting thanks!

@bitinn looks like this only awaits your review now, would be really nice to merge this.

As a side note: in Chrome dev tools, when one selects "copy as Node.js fetch", the request is copied with referrer and referrerPolicy options which doesn't work "as is" but will work once this gets merged.

@tekwiz
Copy link
Member Author

@tekwiz tekwiz commented Oct 7, 2021

@jimmywarting I had to resolve some merge conflicts and fix some errors, but this is ready to merge now. To follow good form, would you mind giving it one more once-over and merge if it looks good?

@jimmywarting
Copy link
Collaborator

@jimmywarting jimmywarting commented Oct 7, 2021

will take a 2nd look at it

@jimmywarting
Copy link
Collaborator

@jimmywarting jimmywarting commented Oct 7, 2021

looks good to me

LinusU
LinusU approved these changes Oct 7, 2021
Copy link
Member

@LinusU LinusU left a comment

LGTM 👍

@LinusU
Copy link
Member

@LinusU LinusU commented Nov 5, 2021

Thanks for the amazing work!

@LinusU LinusU merged commit 2d80b0b into node-fetch:main Nov 5, 2021
9 checks passed
@YakovL
Copy link

@YakovL YakovL commented Nov 5, 2021

Great! In which version can we expect the update?

@LinusU
Copy link
Member

@LinusU LinusU commented Nov 5, 2021

Should be version 3.1.0, I don't think that there have been any talks about when to release it, but the sooner the better in my opinion 👍

@jimmywarting
Copy link
Collaborator

@jimmywarting jimmywarting commented Nov 5, 2021

Don't think we have any valuable new features to push out, it's mostly docs, deprecations and other minor things.
would wish for #1314 to be merged, after that i would like to release 3.1.0

it will come when it comes

expect(() => {
const req = new Request('http://example.com', {referrer: 'foobar'});
expect.fail(req);
}).to.throw(TypeError, 'Invalid URL: foobar');
Copy link
Collaborator

@jimmywarting jimmywarting Nov 9, 2021

Choose a reason for hiding this comment

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

now when this is merged and i'm running test locally, this one fails

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants