cds-snc / node-starter-app

Quick start application setup.... because you have to start somewhere.
MIT License
5 stars 3 forks source link

[WIP] Add multiple session types #137

Open CalvinRodo opened 4 years ago

CalvinRodo commented 4 years ago

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:

timarney commented 4 years ago

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

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

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

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.

CalvinRodo commented 4 years ago

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?