matthewmueller / next-cookies

Tiny little function for getting cookies on both client & server with next.js.
368 stars 17 forks source link

switched to universal-cookie, added tests #21

Closed nfriedly closed 4 years ago

nfriedly commented 4 years ago

@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 commented 4 years ago

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: https://github.com/component/cookie/issues/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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

done, thanks @nfriedly!