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

switched to universal-cookie, added tests #21

Merged
merged 1 commit into from
Nov 2, 2019
Merged

Conversation

nfriedly
Copy link
Collaborator

@nfriedly nfriedly commented Nov 1, 2019

@matthewmueller #20 annoyed me and I decided to switch the underlying cookie library to something that seemed more bug-free. It also led to some simplification of the codebase here.

And, then I decided to add tests, which added a lot of complexity, but I think will prove worth it in the long-run.

The tests

  1. Spin up a small next.js app
  2. Run a suite of nightwatch tests in Chrome
    • client- & server-rendered, with and without a cookie
  3. Shut everything down

The app itself can double as an example.

Anyways, this should probably go out as a semver-major, but I wanted your opinion on it first. Also, you should enable travis-ci.org to automatically run the tests now that they're set up.

I tried out github's built-in CI, but the tests expect Chrome to already be installed, and that's not the case in GitHub's environment, so I rolled that back.

Fixes #20

@matthewmueller
Copy link
Owner

matthewmueller commented Nov 2, 2019

Hey @nfriedly! Thanks for your hard work on this one.

#20 seems like it's pretty easy to resolve without switching the underlying library and now that you've added proper tests, we'll catch issues like this in the future ☺️. In fact, I think this issue has addressed it: component/cookie#8

My main concern is that universal-cookie seems quite a bit larger than what we currently have. What's the gzipped size of universal-cookie? If it's not much larger than I'm happy to switch over!

Everything else in this PR looks 💯 !

@nfriedly
Copy link
Collaborator Author

nfriedly commented Nov 2, 2019

I've been not-entirely-pleased with component-cookie for a while, but was putting off finding something else because I didn't want to do the work.

The fact that #20 existed, combined with the amount of work it was to make the component-cookie tests work pushed me over the edge, though.

universal-cookie seems all-around to be better implemented and better tested.

You're right that component/cookie#8 identified the same issue - although the solution proposed there wasn't really a complete solution :(

As for size, universal-cookie is about 0.74KB bigger after compression, which I think is worth it.

Also, FWIW, the tests that I added wouldn't have caught #20. They currently assume that the underlying library works correctly and are intended to test our code here. Although, now that the infrastructure is in place, it would be fairly trivial to add more tests that could catch something like that.

@matthewmueller
Copy link
Owner

As for size, universal-cookie is about 0.74KB bigger after compression, which I think is worth it.

Okay I think this is a fair tradeoff then. Feel free to merge + publish!

@nfriedly nfriedly merged commit 7ef675d into master Nov 2, 2019
@nfriedly nfriedly deleted the universal-cookie branch November 2, 2019 21:31
@nfriedly
Copy link
Collaborator Author

nfriedly commented Nov 2, 2019

Merged and shipped as v2.0.0.

One last request for you: please go to https://travis-ci.org/account/repositories, log in with your github account, and then then flip the switch for this repo?

@matthewmueller
Copy link
Owner

done, thanks @nfriedly!

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.

problem when get cookie has special character
2 participants