🍪`react-cookie` package #1002

cartogram · 2019-09-16T08:31:22Z

Description

This PR adds a @shopify/react-cookie library that gathers cookies from both the server and client. There are three main components to this library, 1) A universal-provider that provides a set of initial cookies and instantiates 2) a manager. This manager provides common methods for getting/ setting cookies both internally, and in the document. 3) Finally, there are 2 common React hooks for operating on the cookies from within a React component.

Demo/ Tophat

You can view a demo here from this branch of @shopify/web-rails-proving-ground

If you want to tophat yourself, pull down this branch and also the cookies branch from web-rails-proving-ground.

dev up in the cookies branch of web-rails-proving-ground
From the quilt repo run:

cp -R packages/react-cookie/src/ ../web-rails-proving-ground/packages/\@shopify/react-cookie
cp -R packages/react-network/src/ ../web-rails-proving-ground/packages/\@shopify/react-network
cp -R packages/react-server/src/ ../web-rails-proving-ground/packages/\@shopify/react-server

Then run dev run in the web-rails-proving-ground repo.

packages/react-cookie/src/hooks.ts

cartogram · 2019-09-16T14:35:24Z

packages/react-cookie/README.md

@@ -0,0 +1,85 @@
+# `@shopify/react-cookie`


Just roughed this out for completeness, but will put some more ❤️ into this Readme either before or immediately after merging.

packages/react-cookie/src/tests/hooks.test.tsx

packages/react-cookie/package.json

marutypes

Some of the stuff going on in the Cookie component looks weird / suspect, and I'd like some rationale for why we rebuilt all of this instead of just hooking up something like universal-cookie to react-network.

Overall this looks pretty close either way.

packages/react-cookie/src/hooks.ts

packages/react-cookie/src/manager.ts

cartogram · 2019-09-17T15:14:37Z

@TheMallen on doing our own thing vs third-party: this library allows us to provide a cookie solution for applications devs that uses consistent patters as with the other quilt packages that also require similar client/server interaction (self-serializer, react-effect, network, etc…) . Going this route also requires no server-side middleware and everything is confined to only this package (and really one component) rather than requiring orchestrated changes across @shopify/react-server, @shopify/sewing-kit, etc.

Less important, there is only one viable third-party solution out there (the universal-cookie/ react-cookie stack) and I wasn’t the biggest fan of the imperative change-listener way that those libraries operate. I wanted to explore a way that we could leverage our own libraries to provide something better.

That said, I get that additional code is a liability (even given this library is pretty tiny) and understand the hesitation to introduce this package.

packages/react-cookie/README.md

packages/react-cookie/src/Cookie.tsx

marutypes

Looking at this again I think we need this to depend on @shopify/react-network or live as part of that if we want it to work holistically.

packages/react-cookie/src/CookieUniversalProvider.tsx

packages/react-cookie/src/manager.ts

packages/react-cookie/src/UniversalCookies.ts

packages/react-cookie/src/context.ts

marutypes

A few questions / changes but this is looking really nice now

packages/react-cookie/README.md

packages/react-cookie/src/CookieUniversalProvider.tsx

packages/react-cookie/src/context.ts

packages/react-cookie/src/UniversalCookies.ts

packages/react-cookie/src/tests/cookie.ts

packages/react-network/src/manager.ts

cartogram · 2019-09-23T15:44:32Z

@TheMallen another look when you get a chance 🙇 Still doing a final comb through all the Readme, but otherwise all changes are in.

marutypes

I still feel a bit weird about where some of this is, @GoodForOneFare what do you think?

packages/react-cookie/README.md

packages/react-cookie/src/server.ts

packages/react-network/CHANGELOG.md

packages/react-cookie/README.md

packages/react-cookie/src/BrowserCookieManager.ts

packages/react-network/src/ServerCookieManager.ts

packages/react-cookie/src/BrowserCookieManager.ts

GoodForOneFare · 2019-09-24T02:28:29Z

packages/react-cookie/src/server.ts

+    Object.entries(cookies).forEach(([cookie, options]) => {
+      const {value, ...cookieOptions} = options;
+
+      ctx.cookies.set(cookie, value, cookieOptions as any);


Could you add none in a PR to https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/cookies/index.d.ts#L91, please?

packages/react-network/src/ServerCookieManager.ts

GoodForOneFare · 2019-09-24T02:31:19Z

packages/react-cookie/src/BrowserCookieManager.ts

+
+export {CookieContext} from './context';
+
+export class BrowserCookieManager {


I guess this is consistent with the rest of Quilt, but I miss the lint rule for "this isn't really a class".

Yeah this could easily just be a constant object exported directly since it doesn't have any local state. Doesn't matter too much.

.vscode/settings.json

packages/react-cookie/server.d.ts

cartogram · 2019-09-26T00:46:06Z

@TheMallen @GoodForOneFare All changes have been addressed. I also included:

A react-server test that sending cookies with the request will be accessible via the client-side hooks.
Added the UniversalCookieProvider to our default providers
Tests for ServerCookieProvider
Tests for the react-network /server component
More docs for all the changed things
Utils for creating and clearing cookies in tests
probably much more that I am forgetting

The only way I was able to tophat this successfully was by building each library (react-server, react-cookie and react-network) and copying them into web-rails-proving-ground's node_modules. There is a branch there called cookies that has some application code for testing the hooks in this package.

Thanks again for your continual feedback and patience!

marutypes

A few nits but this looks great IMO.

marutypes · 2019-09-26T15:46:58Z

packages/react-cookie/src/BrowserCookieManager.ts

+
+export {CookieContext} from './context';
+
+export class BrowserCookieManager {


Yeah this could easily just be a constant object exported directly since it doesn't have any local state. Doesn't matter too much.

marutypes · 2019-09-26T15:48:35Z

packages/react-cookie/src/hooks.ts

+    manager.setCookie(key, {value, ...options});
+  };
+
+  return [cookie, setCookie];


Personally I like the consistency with koa of doing set('name', null) as long as it's documented

marutypes · 2019-09-26T15:50:59Z

packages/react-cookie/src/tests/CookieUniversalProvider.test.tsx

+  }
+
+  it('provides a universal cookie manager for the server', () => {
+    hasDocumentCookie.mockImplementation(false);


we might want to add a 'why' comment explaining why we have to mock this (so that we can test the server implementation despite jest always having a document.cookie defined)

marutypes · 2019-09-26T15:52:00Z

packages/react-cookie/src/tests/hooks.test.tsx

+        <MockComponent cookie="foo" setValue={setValue} />,
+      );
+
+      wrapper.find('button')!.trigger('onClick');


this spacing is a bit weird IMO

marutypes · 2019-09-26T15:52:49Z

packages/react-cookie/README.md

+    </>
+  );
+}
+```


Can we also document the test utilities we export?

GoodForOneFare

Did not 🎩 but it looks like this should work 👍 😅

GoodForOneFare · 2019-09-26T16:31:47Z

packages/react-cookie/src/hooks.ts

+    manager.setCookie(key, {value, ...options});
+  };
+
+  return [cookie, setCookie];


If it makes sense to follow our rule, I guess it's fine. I'm not 100% sure about how strict we about following it.

Is there any kind of type-hint we can provide like:

export const UNSET = ''; ... setCookie(name: string, value: string | CookieValue | UNSET)

Eww :/

cartogram changed the title ~~react-cookie-package~~ react-cookie package Sep 16, 2019

This was referenced Sep 16, 2019

Universal Cookies #935

Closed

Add cookie middleware and default providers to webpack plugin generated files #941

Closed

cartogram requested a review from marutypes September 16, 2019 14:33

cartogram commented Sep 16, 2019

View reviewed changes

packages/react-cookie/src/hooks.ts Show resolved Hide resolved

cartogram commented Sep 16, 2019

View reviewed changes

packages/react-cookie/src/tests/hooks.test.tsx Outdated Show resolved Hide resolved

cartogram requested a review from GoodForOneFare September 16, 2019 14:37

cartogram commented Sep 16, 2019

View reviewed changes

packages/react-cookie/package.json Show resolved Hide resolved

cartogram changed the title ~~react-cookie package~~ 🍪react-cookie package Sep 16, 2019

marutypes suggested changes Sep 16, 2019

View reviewed changes

packages/react-cookie/src/hooks.ts Show resolved Hide resolved

packages/react-cookie/src/hooks.ts Outdated Show resolved Hide resolved

packages/react-cookie/src/manager.ts Outdated Show resolved Hide resolved

GoodForOneFare reviewed Sep 17, 2019

View reviewed changes

packages/react-cookie/README.md Outdated Show resolved Hide resolved

packages/react-cookie/src/Cookie.tsx Outdated Show resolved Hide resolved

packages/react-cookie/src/Cookie.tsx Outdated Show resolved Hide resolved

cartogram force-pushed the react-cookie branch 2 times, most recently from 429941c to b6b8c6d Compare September 18, 2019 03:40

cartogram requested a review from marutypes September 18, 2019 15:30

marutypes suggested changes Sep 18, 2019

View reviewed changes

packages/react-cookie/src/CookieUniversalProvider.tsx Outdated Show resolved Hide resolved

packages/react-cookie/src/CookieUniversalProvider.tsx Outdated Show resolved Hide resolved

packages/react-cookie/src/manager.ts Outdated Show resolved Hide resolved

cartogram commented Sep 20, 2019

View reviewed changes

packages/react-cookie/src/UniversalCookies.ts Outdated Show resolved Hide resolved

cartogram commented Sep 20, 2019

View reviewed changes

packages/react-cookie/src/UniversalCookies.ts Outdated Show resolved Hide resolved

cartogram commented Sep 20, 2019

View reviewed changes

packages/react-cookie/src/context.ts Outdated Show resolved Hide resolved

cartogram requested a review from marutypes September 20, 2019 17:10

marutypes suggested changes Sep 20, 2019

View reviewed changes

cartogram force-pushed the react-cookie branch 2 times, most recently from f26304d to 4246229 Compare September 23, 2019 14:35

cartogram requested a review from marutypes September 23, 2019 15:44

marutypes suggested changes Sep 23, 2019

View reviewed changes

packages/react-cookie/README.md Outdated Show resolved Hide resolved

packages/react-cookie/README.md Outdated Show resolved Hide resolved

packages/react-cookie/src/server.ts Outdated Show resolved Hide resolved

GoodForOneFare suggested changes Sep 24, 2019

View reviewed changes

cartogram force-pushed the react-cookie branch 2 times, most recently from 969324c to a2eb6aa Compare September 24, 2019 21:54

cartogram force-pushed the react-cookie branch 6 times, most recently from 3b12aea to 9bef59d Compare September 25, 2019 01:10

cartogram requested review from marutypes and GoodForOneFare September 25, 2019 20:56

marutypes approved these changes Sep 26, 2019

View reviewed changes

GoodForOneFare approved these changes Sep 26, 2019

View reviewed changes

Matt Seccafien added 11 commits September 26, 2019 16:38

Adds react-cookie package

6237a7c

readme and changelogs

556edd5

Gord changes

bd15e46

fix up server stuff and tests

d46701b

add any for cookie type

1ce5bd1

more docs

a00438c

typo

a76162e

server tests

81dfb15

better server tests

072e926

add utilities to readme

e632742

few final minor

ce89608

cartogram force-pushed the react-cookie branch from 198940d to ce89608 Compare September 26, 2019 21:34

cartogram merged commit 0bc5704 into master Sep 27, 2019

cartogram deleted the react-cookie branch September 27, 2019 04:25

kvendrik mentioned this pull request Jun 16, 2020

[react-server] remove cookie pass through #1513

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🍪`react-cookie` package #1002

🍪`react-cookie` package #1002

cartogram commented Sep 16, 2019 •

edited

Loading

cartogram Sep 16, 2019

marutypes left a comment •

edited

Loading

cartogram commented Sep 17, 2019

marutypes left a comment

marutypes left a comment

cartogram commented Sep 23, 2019

marutypes left a comment

GoodForOneFare Sep 24, 2019

GoodForOneFare Sep 24, 2019

marutypes Sep 26, 2019

cartogram commented Sep 26, 2019 •

edited

Loading

marutypes left a comment

marutypes Sep 26, 2019

marutypes Sep 26, 2019

marutypes Sep 26, 2019

marutypes Sep 26, 2019

marutypes Sep 26, 2019

GoodForOneFare left a comment

GoodForOneFare Sep 26, 2019


		export {CookieContext} from './context';

		export class BrowserCookieManager {

🍪react-cookie package #1002

🍪react-cookie package #1002

Conversation

cartogram commented Sep 16, 2019 • edited Loading

Description

Demo/ Tophat

Choose a reason for hiding this comment

marutypes left a comment • edited Loading

Choose a reason for hiding this comment

cartogram commented Sep 17, 2019

marutypes left a comment

Choose a reason for hiding this comment

marutypes left a comment

Choose a reason for hiding this comment

cartogram commented Sep 23, 2019

marutypes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cartogram commented Sep 26, 2019 • edited Loading

marutypes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GoodForOneFare left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

🍪`react-cookie` package #1002

🍪`react-cookie` package #1002

cartogram commented Sep 16, 2019 •

edited

Loading

marutypes left a comment •

edited

Loading

cartogram commented Sep 26, 2019 •

edited

Loading