fix: s3 upload index.html at last #12852
Conversation
Codecov Report
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. @@ Coverage Diff @@
## dev #12852 +/- ##
===========================================
+ Coverage 0.00% 48.51% +48.51%
===========================================
Files 1296 842 -454
Lines 149743 38091 -111652
Branches 1296 7755 +6459
===========================================
+ Hits 0 18480 +18480
+ Misses 148447 18021 -130426
- Partials 1296 1590 +294
... and 1258 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
|
||
| await Promise.all(fileList.map(async filePath => { | ||
| if(!filePath.includes('index.html')) { | ||
| return await uploadFileTasks.push(() => uploadFile(s3Client, hostingBucketName, distributionDirPath, filePath, hasCloudFront)); | ||
| }})); | ||
|
|
||
| fileList.filter(filePath => filePath.includes('index.html')).forEach(filePath => { | ||
| uploadFileTasks.push(() => uploadFile(s3Client, hostingBucketName, distributionDirPath, filePath, hasCloudFront)); |
There was a problem hiding this comment.
I suggest to refactor this into two sections for better readability. something like
const indexFile = ... select index from fileList...;
const otherFiles = ... select all other files from fileList....;
await Promise.all(...upload all files...);
await upload(index);
packages/amplify-category-hosting/lib/S3AndCloudFront/helpers/file-uploader.js
Outdated
Show resolved
Hide resolved
| await Promise.all(filesToUploadFirst.map(async filePath => { | ||
| return uploadFileTasks.push(() => uploadFile(s3Client, hostingBucketName, distributionDirPath, filePath, hasCloudFront)); | ||
| })); |
There was a problem hiding this comment.
I missed this the first time, but please move this inside the try/catch after the spinner starts so customers can see something is uploading
There was a problem hiding this comment.
I think it doesn't matter for the user because sequential runs after the spinner and we have to push to the uploadFileTasks first
There was a problem hiding this comment.
I did not notice that sequential before and that we're accumulating deferred () => uploadFile callbacks. Sorry about that.
If actual upload happens in sequential and we'll never parallelize it then perhaps we should just make sure fileList has elements in right order.
There was a problem hiding this comment.
Consider just concatenating filesToUploadFirst.concat(filesToUploadLast) and feeding this to logic that was here before. We don't need two mappings nor Promise.all.
There was a problem hiding this comment.
That's a good point. I just noticed sequential when making the previous comment 😂
Now I wonder if sequential is necessary. What's the purpose of sequential? Would it make the load too slow?
If we want to control the concurrency, would it be better to use promise-pool instead?
| @@ -34,6 +32,13 @@ async function run(context, distributionDirPath) { | |||
| } | |||
| } | |||
|
|
|||
| function sortUploadFiles(fileList) { | |||
| const filesToUploadLast = "index.html"; | |||
| const sortFiles = (fileA, fileB) => fileA.includes(filesToUploadLast) ? 1 : fileB.includes(filesToUploadLast) ? -1 : 0; | |||
There was a problem hiding this comment.
I'll add prettier in another PR
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
packages/amplify-category-hosting/lib/S3AndCloudFront/helpers/file-uploader.js
Fixed
Show resolved
Hide resolved
| exportForTesting: { | ||
| sortUploadFiles, | ||
| } |
There was a problem hiding this comment.
we should not do this. I.e. don't open internals.
instead we should mock modules that are imported into uploader and assert order of s3Client.upload calls.
| fs.createReadStream = jest.fn(() => { | ||
| return {}; | ||
| }); | ||
| mime.lookup = jest.fn(() => { | ||
| return 'text/plain'; | ||
| }); | ||
| await fileUploader.run(mockContext, 'mockDistributionFolder'); |
There was a problem hiding this comment.
the "functionality under test" should not be in beforeAll block, that block is reserved for test setup.
Readability of the tests (i.e. right call in right block) should take precedence over duplication.
Description of changes
This PR is to mitigate #2087 as proposed by @timoteialbu .
Issue: since
index.htmlis not always uploaded to S3 at last, it sometimes causes file not found issue if a dependent file is uploaded after index.htmlFix:
await Promise.allall other files exceptindex.html, then uploadindex.htmlat the end.Note:
index.html. The build tools I checked are: angular-cli@16, react@18.2.0 + cra@5.0.1, react + next@13.4.7, react + vite@4.3.9, vue@3.3.4, vue + nuxt@3.5.2, vue + vite@4.3.9Issue #, if available
#2087
Description of how you validated changes
Checklist
yarn testpassesBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.