Skip to content
This repository has been archived by the owner on Nov 27, 2020. It is now read-only.

[WIP] Add multiple session types #137

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

CalvinRodo
Copy link
Member

We had the need to switch between multiple session types.

Cookies make it easier for us to debug a specific page, Memory makes it easier for us to debug lots of session data, and eventually we'll want to throw a session store like redis or etcd into the mix.

So I added a way to switch the session at compile time.

I'm not sure the best way to configure this so I'm open to ideas at the moment at compile time works for us but might want to eventually use a config file or an environment variable to manage it.

The memory session code was just pulled straight from the 2620 project with just one change to make setting session to undefined actually delete the session.

Changes included:

  • App.js now just adds session configured in session.config.js
  • Added express-session to deal with the problem with long cookies
  • At the moment selecting the session is done through a code change I just
  • can't think of a better place for it right now

App.js now just adds session configured in session.config.js
Added express-session to deal with the problem with long cookies
At the moment selecting the session is done through a code change I just
can't think of a better place for it right now
@timarney timarney temporarily deployed to cds-node-starter-pr-137 November 27, 2019 18:03 Inactive
@timarney timarney temporarily deployed to cds-node-starter-pr-137 November 27, 2019 19:33 Inactive
@CalvinRodo CalvinRodo changed the title Add multiple session types [WIP] Add multiple session types Nov 27, 2019
@timarney
Copy link
Member

A) Been waiting to either have time or have someone submit a PR with the memory store fix.

Have concerns about keeping the broken cookie code around as a few users downstream have needed to make this exact fix (CRA had the original fix).

Personally I use a VS Code debug session + breakpoint but that won't fit all use cases. What type of debugging are you doing with the Cookie? Just looking at saved values?

@jneen
Copy link
Contributor

jneen commented Nov 28, 2019

Would we be able to meet the debugging concerns with an environment variable that causes the url and session to be dumped to the console?

@CalvinRodo
Copy link
Member Author

CalvinRodo commented Nov 30, 2019

So reading my initial PR I realize I didn't really explain the reasons for needing cookie-session still so here it is:

We have a use case in CPP-D Medical Report where we need to add several items to the form, and dynamically generate some data based on those items.

So for instance you add multiple medical conditions and then you get redirected to a page that displays the list of medical conditions and further on you can link treatments and medications to those medical conditions.

You can see the flow here: https://cppdmedicalreport-appservice.azurewebsites.net/en/conditions/add

With the memory session it becomes a bit of work to test those pages since you have to go through earlier forms to generate that data so you can do things like style pages, add in functionality, etc..

In the CPP-D Medical Report we have this functionality swappable using feature flags. https://github.com/cds-snc/cppd-medical-report/blob/master/config/session.config.js

Since we have the need for both iterating quickly on a page with dynamic data, and we have a fairly long form that can blow past the 4096 bytes that a cookie can store.

Ultimately we'd like to add in other session store functionality as well, and have it be configurable depending on the context of the work that you are doing.

I'm also open to other suggestions for solving the problems we are running into.

@timarney
Copy link
Member

timarney commented Dec 2, 2019

It's been a while since I looked at options for "local storage"

Perhaps using something like one of these would be better than using the Cookie store where we know we're going to top out for file size.

Want to keep this simple while covering off that sort of use case.

https://github.com/js-data/js-data
https://github.com/localForage/localForage

Also wondering if creating a type of fixture that could be pulled in ... I want to test / code for this scenario which would all setting up form defaults would be a good approach.

Removed Cookie Session
Added a file based session store
Standardized on session config other then backer
@CalvinRodo
Copy link
Member Author

Okay fair enough, I replaced the cookie store with a file based store instead, it also uses the same API as memory session which I think is a bit nicer.

I'm still open to suggestions on switching, in CPP-D we've implemented feature flags for this would it make sense to bring that upstream?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants