Skip to content
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

Feature/do 1357 cdk v2 templating #1395

Merged
merged 10 commits into from
Sep 10, 2024
Merged

Conversation

crispy101
Copy link
Contributor

@crispy101 crispy101 commented Aug 22, 2024

Description of the proposed changes

  • address Invalid define value error - https://esbuild.github.io/api/#define
  • OriginRequest and Cache Header update
  • enhance readability in code and version numbers
  • static-hosting release: 2.4.0

Screenshots (if applicable)

Other solutions considered (if any)

Notes to PR author

⚠️ Please make sure the changes adhere to the guidelines mentioned here

Notes to reviewers

🛈 When you've finished leaving feedback, please add a final comment to the PR tagging the author, letting them know that you have finished leaving feedback

@crispy101 crispy101 requested review from TheOrangePuff and a user August 22, 2024 02:11
@crispy101 crispy101 changed the title Feature/do 1357 cd kv2 templating Feature/do 1357 cdk v2 templating Aug 22, 2024
Copy link
Contributor

@TheOrangePuff TheOrangePuff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few formatting comments

@@ -33,7 +33,7 @@ export class PathRemapFunction extends Construct {
local: new Esbuild({
entryPoints: [join(__dirname, "handlers/remap.ts")],
define: {
"process.env.REMAP_PATH": options.path,
"process.env.REMAP_PATH": '"' + options.path + '"',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be changed to:

"process.env.REMAP_PATH":  `"${options.path}"`,

I think it's just a little cleaner

packages/static-hosting/lib/static-hosting.ts Show resolved Hide resolved
"x-forwarded-host", // Origin response may vary depending on the domain/path based on Feature Environment
"x-prerender", // Origin response may vary depending on whether the request is from end user or prerender service.
];
if (props.additionalDefaultCacheKeyHeaders) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment about the spread operator here

Copy link
Contributor

@TheOrangePuff TheOrangePuff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@crispy101 crispy101 merged commit 1c90151 into main Sep 10, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants