search

aws-amplify / amplify-cli

The AWS Amplify CLI is a toolchain for simplifying serverless web and mobile development.
Apache License 2.0
2.82k stars 818 forks source link

Uploading the files of the web app to S3 should be in a different order #2087

Open timoteialbu opened 5 years ago

timoteialbu commented 5 years ago

Note: If your issue/bug is regarding the AWS Amplify Console service, please log it in the Amplify Console GitHub Issue Tracker

Describe the bug We get the following error when deploying our app: image

This error happens for a brief 15-30 seconds, but its enough to give us less than 100% uptime.

The reason this happens is because of the order that files are uploaded in. Basically the index.html file is uploaded before the scripts that it requires are, so when a user refreshes the page in that upload time frame, the index.html file they receive references a script that is not in S3 yet and therefore it throws the error above.

I added a console log in here https://github.com/aws-amplify/amplify-cli/blob/master/packages/amplify-category-hosting/lib/S3AndCloudFront/helpers/file-uploader.js#L65 and was able to observe this upload order:

image

To Reproduce Steps to reproduce the behavior:

  1. Upload the app.
  2. This is a little harder to see if your internet is blazing fast, but if you can throttle it a little, then try to reload the page right after the index.html file is uploaded and you will see the above behavior.

Expected behavior I expect the index.html to be uploaded last so that all the resources it needs are available to it. Its not about the order above, as promise-sequential which is a wrapper around Promise.all does not guarantee the order the promises are fulfilled in. So you probably need to upload everything else first and once that succeeds, upload the index.html file. I realize as I am typing this that maybe a non-react app while have a different entry point potentially, so maybe this is not the ideal solution for all use cases.

Screenshots See above.

Desktop (please complete the following information):

Smartphone (please complete the following information): Did not test as this is not a browser/OS specific issue.

Additional context This is a reactjs app.

Is this something I could work on in case its not a priority? I would be more than happy to contribute as we have been using amplify-cli extensively at my company since December 2018.

attilah commented 5 years ago

@timoteialbu Thanks for reporting the issue looking at the code briefly and I saw that there is no special ordering being during file scan, extra handling for the index document should be added there here.

Would you be interested submitting a PR with test for this issue?

UnleashedMind commented 5 years ago

@timoteialbu Thanks for pointing this out. But I will re-label this as an enhancement instead of a bug. The CLI hosting feature uses a general approach for different frontend frameworks, and different application update workflows. Different updates can change different parts of your application, and if there's a mismatch between the index file and other files that it refers to due to an update, you will see this behavior. So uploading the index file last wouldn't resolve this issue in general. IMHO, the correct approach would be to display a page temporarily during an update, notifying the user to come back again after a few minutes util your app is completely updated in the hosting bucket.

0618 commented 1 year ago

Hi @timoteialbu ! We have a PR to mitigate your issue #12852 , but it's not ideal. We would suggest you to cache and versionize your app.