Skip to content

Amend downloadFiles loop to a Promise.all to ensure writes are complete#6863

Draft
EvilGenius13 wants to merge 1 commit intomainfrom
update-theme-downloader-file-download
Draft

Amend downloadFiles loop to a Promise.all to ensure writes are complete#6863
EvilGenius13 wants to merge 1 commit intomainfrom
update-theme-downloader-file-download

Conversation

@EvilGenius13
Copy link
Contributor

WHY are these changes introduced?

The current forEach loop in downloadFiles() doesn't await async callbacks. This causes the function to return immediately while file writes are still in progress. The CLI will show 100% progress even though files may still be writing to disk, and if there are any errors, they won't surface since the function has already completed.

WHAT is this pull request doing?

Replaces forEach(async ...) with Promise.all() to properly await all file writes. If there is an error, it will properly propagate up.

How to test your changes?

  • Build the branch
  • Run theme pull and ensure downloads still work and the CLI terminal output works.

You can also run this handy script to see how forEach doesn't await in the loop.

Details ``` const POKEMON_IDS = [1, 4, 7, 25, 133];

async function fetchPokemon(id) {
const response = await fetch(https://pokeapi.co/api/v2/pokemon/${id});
const data = await response.json();
return data.name;
}

function timestamp() {
return [${new Date().toISOString().split("T")[1].slice(0, -1)}];
}

async function withForEach() {
console.log(\n--- forEach ---);
console.log(${timestamp()} forEach started);

POKEMON_IDS.forEach(async (id) => {
const name = await fetchPokemon(id);
console.log(${timestamp()} fetched: ${name});
});

console.log(${timestamp()} forEach complete\n);
}

async function withPromiseAll() {
console.log(\n--- Promise.all ---);
console.log(${timestamp()} Promise.all started);

await Promise.all(
POKEMON_IDS.map(async (id) => {
const name = await fetchPokemon(id);
console.log(${timestamp()} fetched: ${name});
})
);

console.log(${timestamp()} Promise.all complete\n);
}

async function main() {
await withForEach();
// give the orphaned forEach fetches time to print
await new Promise((resolve) => setTimeout(resolve, 3000));
await withPromiseAll();
}

main().catch(console.error);

</details>
### Post-release steps

<!--
  If changes require post-release steps, for example merging and publishing some documentation changes,
  specify it in this section and add the label "includes-post-release-steps".
  If it doesn't, feel free to remove this section.
-->

### Measuring impact

How do we know this change was effective? Please choose one:

- [ ] n/a - this doesn't need measurement, e.g. a linting rule or a bug-fix
- [ ] Existing analytics will cater for this addition
- [ ] PR includes analytics changes to measure impact

### Checklist

- [ ] I've considered possible cross-platform impacts (Mac, Linux, Windows)
- [ ] I've considered possible [documentation](https://shopify.dev) changes

@github-actions
Copy link
Contributor

Coverage report

St.
Category Percentage Covered / Total
🟡 Statements 78.9% 14556/18449
🟡 Branches 73.23% 7223/9863
🟡 Functions 79.09% 3704/4683
🟡 Lines 79.25% 13762/17366

Test suite run success

3777 tests passing in 1455 suites.

Report generated by 🧪jest coverage report action from efa913d

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.

1 participant

Comments