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.

Sign up for GitHub

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

Jump to bottom

feat(ssr): support custom link header #4161

Closed
wants to merge 3 commits into from
Closed

feat(ssr): support custom link header #4161

wants to merge 3 commits into from

Conversation

tillkruss
Copy link

Types of changes

  • Bug fix (a non-breaking change which fixes an issue)
  • New feature (a non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Description

It's currently impossible to overwrite or extend the Link header sent by Nuxt.

We have a middleware that needs to add a couple of preconnect links:

<https://cdn.example.com>; rel=preconnect; crossorigin

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly. (PR: #)
  • I have added tests to cover my changes (if not applicable, please state why)
  • All new and existing tests are passing.

tillkruss
tillkruss commented Oct 21, 2018
View reviewed changes
packages/core/src/middleware/nuxt.js
// Pass with single Link header
// https://blog.cloudflare.com/http-2-server-push-with-multiple-assets-per-link-header
// https://www.w3.org/Protocols/9707-link-header.html
res.setHeader('Link', pushAssets.join(','))
res.setHeader('Link', pushAssets.join(', '))
Copy link
Author

@tillkruss tillkruss Oct 21, 2018 •
edited

Choose a reason for hiding this comment

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

We're already using spaces after semicolons in line 64 so we might as well keep it uniform.

@manniL manniL requested review from pi0 and clarkdo October 21, 2018 01:02
@codecov-io
Copy link

codecov-io commented Oct 21, 2018 •
edited

Codecov Report

Merging #4161 into dev will decrease coverage by 0.04%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev    #4161      +/-   ##
==========================================
- Coverage   88.84%   88.79%   -0.05%     
==========================================
  Files          37       37              
  Lines        1649     1651       +2     
  Branches      431      432       +1     
==========================================
+ Hits         1465     1466       +1     
- Misses        157      158       +1     
  Partials       27       27
Impacted Files Coverage Δ
packages/core/src/middleware/nuxt.js 98.41% <66.66%> (-1.59%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4b7afcc...71176c1. Read the comment docs.

manniL
manniL approved these changes Oct 21, 2018
View reviewed changes
Copy link
Member

@manniL manniL left a comment

Choose a reason for hiding this comment

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

LGTM!

@awronski
Copy link

Hi Guys!

What do you think about this one?
https://cmty.app/nuxt/nuxt.js/issues/c7870

@manniL
Copy link
Member

manniL commented Oct 21, 2018

@awronski Of course, implementing #4011 and giving the user the choice would open up more possibilities!

A PR would be appreciated 👏

@awronski
Copy link

@manniL ok, I will try to do PR for #4011.

@tillkruss
Copy link
Author

@awronski: I'd prefer a function, too.

clarkdo
clarkdo reviewed Oct 21, 2018
View reviewed changes
Copy link
Member

@clarkdo clarkdo left a comment

Choose a reason for hiding this comment

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

Good idea and if you can add test for it, it will be perfect then😊

@galvez galvez changed the title Support custom Link header feat(ssr): support custom link header Oct 21, 2018
galvez
galvez approved these changes Oct 21, 2018
View reviewed changes
Copy link

@galvez galvez left a comment

Choose a reason for hiding this comment

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

LGTM

pi0
pi0 approved these changes Oct 22, 2018
View reviewed changes
Copy link
Member

@pi0 pi0 left a comment

Choose a reason for hiding this comment

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

LGTM. Adding a test can make it a perfect one!

@tillkruss
Copy link
Author

@pi0, @clarkdo: I'd love to add a test, but I have to admit I wouldn't even know where to start.

@galvez
Copy link

galvez commented Oct 23, 2018

@tillkruss I'll take care of it :)

@tillkruss
Copy link
Author

We can close this in favour of #4198 IMO.

@manniL
Copy link
Member

manniL commented Oct 25, 2018

closed in favor of #4198

@manniL manniL closed this Oct 25, 2018
pi0 pushed a commit that referenced this pull request Oct 26, 2018
@awronski @pi0
feat(core): custom pushAssets function to control http2 push headers (#…
c9fd780 
…4198)

Resolves #4011
Resolves #4161
@lock
Copy link

lock bot commented Nov 24, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Nov 24, 2018
@danielroe danielroe added the 2.x label Jan 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@clarkdo clarkdo clarkdo left review comments

@galvez galvez galvez approved these changes

@manniL manniL manniL approved these changes

@pi0 pi0 pi0 approved these changes

Assignees

@galvez galvez

@tillkruss tillkruss

Labels
2.x
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@tillkruss @codecov-io @awronski @manniL @galvez @clarkdo @pi0 @danielroe