-
Notifications
You must be signed in to change notification settings - Fork 1
Conversation
? router.push(pathname, as, { query: { search: searchParam } }) | ||
? router.push( | ||
{ pathname, query: as ? {} : { search: searchParam } }, | ||
as && { pathname: as, query: { search: searchParam } } | ||
) |
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.
This is slightly unrelated to this PR. I broke some search functionality in my last set of changes, so this fixes it.
@@ -41,6 +43,7 @@ export const authorizedPage = ( | |||
): GetServerSideProps => { | |||
return async (context: Context) => { | |||
if (await verifyServerSideAuthorization(context)) { | |||
context.res.setHeader("Cache-Control", "no-cache"); |
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.
These are safety valves so that our auth'd routes don't accidentally get cached by the CDN. I think they're fine to be cached by the browser (which is why I didn't add no-store
).
@@ -59,7 +61,7 @@ export const verifyServerSideAuthorization = async (context: Context) => { | |||
res.writeHead(302, { | |||
Location: `https://stagingapi.artsy.net/oauth2/authorize?client_id=${ | |||
process.env.APP_ID | |||
}&redirect_uri=http://${host}/oauth2/callback?redirect_to=${encodeURI( | |||
}&redirect_uri=${urlFromReq(req)}/oauth2/callback?redirect_to=${encodeURI( |
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 had a whole slew of URLs that only worked on http
or localhost
. This is a unifying way of fixing those.
new Isotope<T>( | ||
{ | ||
domain: `artsy-studio-${name}`, | ||
key: "id" | ||
}, | ||
{ | ||
accessKeyId: process.env.ACCESS_KEY_ID, | ||
secretAccessKey: process.env.SECRET_ACCESS_KEY, | ||
region: process.env.REGION | ||
} | ||
); |
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.
Why this change is required is a little weird.
Now restricts certain environment variables from being included in a build (like say... the AWS access token). Isotopes is just a simple wrapper around SimpleDB which we're using for a little metadata store.
Isotopes doesn't actually support passing these yet, but I've got a PR open to add it. I patched it in w/ patch-package for now.
This adds the ability to do now deployments (doesn't automate it). I want to go ahead and push it through before I accidentally start tacking more changes on top of it 😄