-
Notifications
You must be signed in to change notification settings - Fork 142
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
Add multi-value
param bug fix/work around in SDK
#1134
Conversation
multi-value param
bug fix/work around in SDKmulti-value
param bug fix/work around in SDK
@@ -73,6 +73,29 @@ const logAndFormatError = (err) => { | |||
} | |||
} | |||
|
|||
// Because multi-value params are not supported in `aws-serverless-express` we need to re-add | |||
// the stripped duplicate params. | |||
const FIX_LOCATION_SEARCH = (req, location) => { |
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.
ALL CAP? 🤨
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.
Lol 😆 same as @alexvuong ... Caps is for constants... I would name it fixMePleaseLocationSearch
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 did this intentionally to make it super visible. Doing this will ensure that we remove it once we upgrade the underlying library.
@@ -94,11 +117,15 @@ export const render = async (req, res, next) => { | |||
const WrappedApp = routeComponent(App, false, res.locals) | |||
|
|||
const [pathname, search] = req.originalUrl.split('?') | |||
const location = { | |||
let location = { |
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.
We are changing property inside the location object, const is still valid
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.
Artifact of going to different solution iterations. Fixed it.
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.
Agree with @alexvuong's nits, but not blocking. Thanks for fixing @bendvc! 🙏
Closing because when v3 is released the new version of serverless-express will be used an should fix this issue. Here is the PR for v2 |
Description
We are losing multi-value param values in the runtime because of
aws-serverless-express
as a result of thereq.originalUrl
not being fully preserved. For example a request forhostname.com/path?param=1¶m=2
will eventually be made available in the SDK and down into the PWA-Kit app with only the last param valuehostname.com/path?param=2
.Fortunately this issue doesn't happen to effect the
req.query
property. So as a temporary fix, we'll re-create thelocation.search
value using thereq.query
paying special attention to the order of the params.This fix is meant to be ugly so we remove it in the future!
Types of Changes
Changes
location.search
property in the react rendering pipeline using thereq.params
which has the duplicate param values.How to Test-Drive This PR
General