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

Add single-page-app construct #175

Merged

Conversation

adriencaccia
Copy link
Contributor

Fix #172

@mnapoli
Copy link
Member

mnapoli commented Mar 17, 2022

Thank you for opening this! I'll review this tomorrow!

Just a question because I got lost with #172: is this PR using Lambda@Edge? From a quick search it doesn't seem so.

@adriencaccia
Copy link
Contributor Author

Just a question because I got lost with #172: is this PR using Lambda@Edge? From a quick search it doesn't seem so.

No 😃 !

I managed to make the SPA redirection work by only adding a cloudfront function triggered by the viewer request event.

@mnapoli
Copy link
Member

mnapoli commented Mar 17, 2022

Amazing!

src/constructs/aws/SinglePageApp.ts Outdated Show resolved Hide resolved
src/constructs/aws/SinglePageApp.ts Outdated Show resolved Hide resolved
src/constructs/aws/SinglePageApp.ts Outdated Show resolved Hide resolved
src/constructs/aws/StaticWebsite.ts Outdated Show resolved Hide resolved
src/constructs/aws/abstracts/StaticWebsiteAbstract.ts Outdated Show resolved Hide resolved
src/constructs/aws/SinglePageApp.ts Outdated Show resolved Hide resolved
.vscode/launch.json Show resolved Hide resolved
docs/single-page-app.md Outdated Show resolved Hide resolved
@adriencaccia adriencaccia force-pushed the fix/static-website-404 branch 2 times, most recently from 928ad8c to 3faf11d Compare March 18, 2022 15:14
Copy link
Member

@mnapoli mnapoli left a comment

Choose a reason for hiding this comment

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

Thanks for addressing all this so quickly!

cc @fredericbarthelet if you want to review

I'll try out the component early next week (just to be sure) but except that I'm all good here!

Copy link
Collaborator

@fredericbarthelet fredericbarthelet left a comment

Choose a reason for hiding this comment

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

@adriencaccia thanks for putting this PR together :)

LGTM, except a few minor comments and e5da8c9 that can then be reverted to reduce boilerplate and avoid confusion on which mechanism is in place to redirect to index (in the PR current state, it looks like both Cloudfront and S3 are in charge of the redirect)

@mnapoli
Copy link
Member

mnapoli commented Mar 21, 2022

FYI I just tested deploying the construct and 👌 perfect.

@adriencaccia
Copy link
Contributor Author

@adriencaccia thanks for putting this PR together :)

LGTM, except a few minor comments and e5da8c9 that can then be reverted to reduce boilerplate and avoid confusion on which mechanism is in place to redirect to index (in the PR current state, it looks like both Cloudfront and S3 are in charge of the redirect)

Thanks for the review !

One problem though, reverting e5da8c9 will not be enough, we will also need to add custom OAI as stated in my comment in the corresponding issue.

So I see a tradeoff here between:

  • Managing the custom OAI
  • Setting the bucket in website hosting

The current solution (website hosting) is the one with less boilerplate in my opinion. What do you think ?

@fredericbarthelet
Copy link
Collaborator

@adriencaccia as seen together, reverting back to OAI and private bucket (removing website hosting on S3 bucket) will work as intended with your new Cloudfront function redirecting SPA path to /. This fix + enabling redirectToMainDomain on the new construct and this PR will be good to go :)

Thanks again for your contribution, let me know if @mnapoli and I can help in any way.

@mnapoli
Copy link
Member

mnapoli commented Mar 22, 2022

@adriencaccia as seen together, reverting back to OAI and private bucket (removing website hosting on S3 bucket) will work as intended with your new Cloudfront function redirecting SPA path to /. This fix + enabling redirectToMainDomain on the new construct and this PR will be good to go :)

Thanks again for your contribution, let me know if @mnapoli and I can help in any way.

+1, yes I'd say OAI has a great advantage (private bucket). If we can get these things ☝️ IMO we're good to go!

@adriencaccia
Copy link
Contributor Author

Hey @mnapoli @fredericbarthelet !

I made all the changes we talked about!
The origin bucket is now private. The main change is that users will get a 403 when trying to access a non existing file (I am writing this here so that we are all on the same page).

The construct is working properly on my end, can you test it ?

Copy link
Collaborator

@fredericbarthelet fredericbarthelet left a comment

Choose a reason for hiding this comment

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

@adriencaccia LGTM :) !
Thanks for your contribution. I'll merge and release a new version of Lift right away.

@fredericbarthelet fredericbarthelet merged commit 9aed002 into getlift:master Apr 4, 2022
@mnapoli
Copy link
Member

mnapoli commented Apr 4, 2022

🎉 thanks @adriencaccia!

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.

[Static Website] Getting 404 on nested paths, and security headers are not applied
3 participants