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

feat!: new implementation of sessionCookieStore #7

Merged
merged 7 commits into from
May 9, 2023

Conversation

johannes-lindgren
Copy link
Contributor

@johannes-lindgren johannes-lindgren commented May 5, 2023

What

A new implementation of sessionCookieStore. It is based on the old one, with the following changes:

  • The autoRefresh option in sessionCookieStore.get is removed since we've never used it. This is a breaking change, We can reintrodice it later if we need it.
  • There are four crud operations for getting, setting, and removing app session cookies: get, getAll, put, and remove. The implementation of these are now moved to dedicated files; getSession, getAllSessions, putSession, removeSession.
  • Created a utils module that is only used by the crud module.
  • The utility function matches was renamed to keysEquals and moved to its own file.
  • The utility function toKeys was renamed to keysFromQuery and moved to its own file.
  • getSignedCookie and setSignedCookie were changed so that they no longer depend on Node.js (http module)

Why

In the session module, the only point where there is a dependency on Node.js is in sessionCookieStore.ts. All the other functions accepts functions for reading and writing string values (GetCookie and SetCookie), but they're actually completely agnostic to how these values are stored. This has two benefits that we will be able to leverage in the future:

  • It will become easy to make the library framework agnostic.
  • It will become easy to offer alterative ways of storage. It will be better if the accessToken is securely stored in a database instead of in a cookie (like we do with the deployments apps).

The next step will be to replace grant with our own, custom request handlers.

How to test

Run the Nextjs starter project locally.

yarn dev

Check out this branch and build the project.

yarn build

Link the packages with:

In the library:

yarn link

and in the template

yarn link @storyblok/app-extension-auth

In the browser, clear the cookies and reload the app. Everything should work just as before without any modification to the code.

@johannes-lindgren johannes-lindgren marked this pull request as ready for review May 5, 2023 15:58
src/session/crud/putSession.ts Outdated Show resolved Hide resolved
src/session/crud/removeSession.ts Show resolved Hide resolved
Comment on lines +43 to +45
const newAppSession = await refreshAppSession(refreshToken(fetch)(params))(
currentAppSession,
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this always called from the browser? Or is there a possibility that this can be run on the server side?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Always called from the server

Copy link
Contributor

Choose a reason for hiding this comment

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

ah okay. then don't we need to pass fetch from node-fetch package?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gosh, you're right 🤯

In the very beginning, the library used to have code running in the client as well as on the server. It seems like I've forgotten to purge the tsconfig compilerOptions.lib "dom" option.

The library has only been used by us in a Next which uses the isomorphic-fetch, so we never noticed.

I've opened a new ticket for it here: https://storyblok.atlassian.net/browse/EXT-1531

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! Does the ticket include the replacement of fetch with node-fetch? Or how do you want to approach that part?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure yet... there are a few options:

  • use node-fetch
  • require node version >= 18 (where fetch was added)
  • use openid-client to make the refresh request. When I'm replacing grant, I'll use the openid-client library for making request to the Storyblok API.

Probably the last option here

src/session/sessionCookieStore.ts Outdated Show resolved Hide resolved
johannes-lindgren and others added 3 commits May 9, 2023 15:59
Co-authored-by: Eunjae Lee <hey@eunjae.dev>
Co-authored-by: Eunjae Lee <hey@eunjae.dev>
Comment on lines +14 to +17
const getCookie: GetCookie = (name) =>
getNodeCookie(requestParams.req, name)
const setCookie: SetCookie = (name, value) =>
setNodeCookie(requestParams.res, name, value)
Copy link
Contributor

Choose a reason for hiding this comment

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

So far, from my understanding this sessionCookieStore is still relying on the Node.js request and response object because getNodeCookie and setNodeCookie expects them. And your plan to going to make this replaceable? Is it the changes you told me you're thinking of?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes exactly.

It also makes the other functions easier to test, because we don't need to mock request and response objects; just the getter and setter functions.

Copy link
Contributor

@eunjae-lee eunjae-lee left a comment

Choose a reason for hiding this comment

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

This PR looks good. Let's move onto the next one! :)

@johannes-lindgren johannes-lindgren merged commit b65e017 into main May 9, 2023
1 check passed
@johannes-lindgren johannes-lindgren deleted the EXT-1511-new-session-store branch May 9, 2023 15:27
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

2 participants