Break up the cookies classes #1024
Conversation
|
@dwightwatson the tests failures are related to these changes and it seems that the changes here might have broken the CLI. Try generating a new app with these changes and make sure the app runs successfully. |
|
I like the breakdown of the classes and moving in the right direction. Maybe abstract the duplication of methods |
|
I've merged master in after #1027 and the suite has gone back to green. I've broken out an |
| set(name, value) | ||
| end | ||
|
|
||
| abstract def get(name) |
There was a problem hiding this comment.
I'd use this opportunity to change it to get?. This way we have a free #[]? and a cheaper way to handle non-existent keys.
There was a problem hiding this comment.
If possible, I'd rather tackle that sort of improvement in another PR. The focus of this one was to break up the classes, remove dead code and have it all still work.
Description of the Change
This was done in response to #1014.
ChainedStoremodule is just rolled intoAmber::Router::Cookieswhere it is used.SerializedStoreandJsonSerializerare removed as there don't appear to be references to them.I'm sure there is still work that can be done to cleanup this code, but I think this is a good first step.
Alternate Designs
Leaving it in a single file as it was, or breaking it out to multiple files but keeping the old stuff around as well.
Benefits
It'll be much easier to reason with and work with the cookie related code. It will also make it easier to attempt to DRY up any of the code that can be.
There appear to be test failures - but I'm not sure they are related. Would love a second opinion/point in the right direction. I'm having trouble replicating test failures locally - I don't know if it's something with my setup or something that I'm doing wrong.