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

fix: revert using cookies package to fix cookie headers handling #332

Merged
merged 1 commit into from
Jun 22, 2019

Conversation

rclement
Copy link

PR #327 introduced a significant increase of vendors bundle size, as reported in issue #330.

The root cause was the replacement of the cookie package with the cookies one, while fixing a severe bug in Set-Cookie headers handling for the SSR language redirection cookie.

This PR reverts to using the cookie package but performs manually the Set-Cookie header handling, exactly the way the cookies package does (i.e. no previous cookie, a single cookie or a list of cookies).

Hopefully, this one will do the trick!

@codecov
Copy link

codecov bot commented Jun 22, 2019

Codecov Report

Merging #332 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #332   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           2      2           
  Lines           6      6           
=====================================
  Hits            6      6

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 98afbc7...150eccc. Read the comment docs.

@rclement rclement changed the title Revert using cookies package to fix cookie headers handling fix: revert using cookies package to fix cookie headers handling Jun 22, 2019
@rchl
Copy link
Collaborator

rchl commented Jun 22, 2019

LGTM

@rchl
Copy link
Collaborator

rchl commented Jun 22, 2019

I've squashed this with your previous change locally to see changes relative to pre-cookies-package change and got this simple diff:

diff --git a/src/plugins/main.js b/src/plugins/main.js
index d916954..35f3b34 100644
--- a/src/plugins/main.js
+++ b/src/plugins/main.js
@@ -66,11 +66,18 @@ export default async (context) => {
         path: '/'
       })
     } else if (res) {
+      let headers = res.getHeader('Set-Cookie') || []
+      if (typeof headers == 'string') {
+        headers = [headers]
+      }
+
       const redirectCookie = Cookie.serialize(cookieKey, locale, {
         expires: new Date(date.setDate(date.getDate() + 365)),
         path: '/'
       })
-      res.setHeader('Set-Cookie', redirectCookie)
+      headers.push(redirectCookie)
+
+      res.setHeader('Set-Cookie', headers)
     }
   }

Makes it easier to evaluate changes.

@paulgv
Copy link
Collaborator

paulgv commented Jun 22, 2019

Awesome! Thank you for the PR @rclement! And thank you for posting the diff @rchl! LGTM.

@paulgv paulgv merged commit 8a0ee00 into nuxt-modules:master Jun 22, 2019
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.

None yet

3 participants