Add full ssr cookie support to storage package #496
Conversation
|
|
Would really appreciate if someone would take the time to review this and merge it, so that i don't have to bundle the storage package in my project |
BierDav
changed the title
BierDav
changed the title
|
now everything should look good |
|
@thetarnav can you approve the workflow, i had to change a few things this should know fix the failing tasks |
|
Finally now all tests should pass :) |
|
Sorry, I was out for a longer time. I will rewrite this part soon so the options can be given to a init(options) method. |
|
is there an ETA? We need this feature for our codebase |
|
Btw. i fixed the merge conflicts so ... please merge 🤠 |
- add set-cookie also for reads - improve readability and extensibility - pass options to all storageOperations, because cookieStorage depends on having getRequest
Changed the property name from 'name' to 'key' in Storage Interface to improve code readability. This is consistent with other usages and brings more clarity about what this property actually does.
|
is there an ETA? |
|
Soon-ish. I'll try to get this out before I start on the setup idea. I need to check why the test/build step is failing later. |
|
Sorry, the rebase seems to have removed the import for the Accessor type. @BierDav, could you please fix that on your side, as I cannot edit your branch? Also, PersistenceOptions' second type argument should be defaulted to {}. |
Added 'Accessor' to the imported types from 'solid-js' and altered the function parameters of 'makePersisted' functions. The 'PersistenceOptions' now include an empty object in their type definitions.
|
Fixed 👍 |
|
Merged. It should be released soon. |
|
When can we expect the release? btw. is there a chance of getting a alpha version? |
Before there were issues when just setting a cookie using storage api on server and you set a custom setCookie it fails when it tries to raise an Event on server, because the URL api is only available on the client.
Additionally i have added full support for the "Set-Cookie" header so that when the server sets a cookie this automatically gets passed to the client. This only works if a PageEvent is returned by getRequest, because i obviously have to access the responseHeaders. I tried to make everything as backwards compatible as possible.
There is one thing to think about in the future, cookies are not only identified by their key, but by the combination of key, path and domain. The code currently and before assumed that key is the only primary identifier.
Now the StorageOptions should be passed to every storage method, when possible, because cookiestorage requires the getRequest method to access responseHeaders on vite. This is a known limitation of vite that we cannot dynamically import libraries therefore vite users (as me) must set the getRequest option manually. This is documented!
When reading cookies responseHeaders is obviously also checked so that the server can set the cookie without having to deal with a wrong get value.