Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat!: new implementation of sessionCookieStore #7
Changes from all commits
a4b7573
9e0d485
0a78119
8ed8460
afd812d
26ea576
0b2c361
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
This file was deleted.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
fromnode-fetch
package?There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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:
Probably the last option here
There was a problem hiding this comment.
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 becausegetNodeCookie
andsetNodeCookie
expects them. And your plan to going to make this replaceable? Is it the changes you told me you're thinking of?There was a problem hiding this comment.
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.