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 error messaging for if a file fails to download #481

Merged
merged 1 commit into from
Apr 29, 2021
Merged

Conversation

drewjenkins
Copy link
Contributor

Description and Context

Fixes #450

This was a bit tricky to repro, but I was able to do so by manually failing the request by manipulating my wifi via a script like

const { exec } = require('child_process');

exec('rm -rf temp/images', () => {
  console.log('removed');

  console.log('starting fetch');
  exec(
    './node_modules/.bin/hs fetch modules temp/images',
    (err, stdout, stderr) => {
      console.log(stdout, stderr);
      console.log('completed fetch');
      exec('networksetup -setairportpower Wi-Fi on', () => {
        console.log('wifi turned on');
      });
    }
  );

  setTimeout(() => {
    console.log('turning off wifi');
    exec('networksetup -setairportpower Wi-Fi off', () => {
      console.log('wifi turned off');
    });
  }, 1300);
});

With this fix, we will now show an error message instead of a success message if any of the files fails to download

@drewjenkins drewjenkins requested review from gcorne and anthmatic April 29, 2021 16:01
Copy link
Contributor

@brandenrodgers brandenrodgers left a comment

Choose a reason for hiding this comment

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

This lgtm 👍 Just two questions

Does logFileSystemErrorInstance not log the errors (in writeFileMapperNode)?
Also, could you post a screenshot of what the failed state looks like now?

@drewjenkins
Copy link
Contributor Author

It logs the errors, but the whole operation is marked success as long as it completes, regardless if it has errors or not. And sure. My output is a bit different than above just cuz of how I was reproing, also it isnt colored cuz im running it thru a node script, but here

@brandenrodgers

image

@drewjenkins drewjenkins merged commit 353faed into master Apr 29, 2021
@drewjenkins drewjenkins deleted the issues/450 branch April 29, 2021 17:37
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.

Better message when network error occurs
2 participants