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

Fail string errors silently #156

Open
wants to merge 2 commits into
base: MOODLE_402_STABLE
Choose a base branch
from

Conversation

sarahjcotton
Copy link

Fix for issue #155

@@ -199,7 +198,7 @@ define(

});
}).catch(() => {
Notification.exception(new Error('Failed to load string: loading'));

Choose a reason for hiding this comment

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

Is there any info on the root cause in general? I'm guessing this leads to a bit of broken UI as well, that might be worth considering if its a somewhat frequent issue. If this is just network failures that are random, this is likely fine.

Also, if we are removing the notifications, I dont think we need the catchs at all, and they can just be removed?

Copy link
Author

Choose a reason for hiding this comment

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

I haven't been able to identify the cause but I have seen it occur pretty frequently. That said, the way the missing string is handled seemed to cause more of a problem than the missing string and I don't rememeber observing any issues beyond the error keep popping up.

You're probably right about removing the catches so I'll take another look. It's a been a while so fresh eyes might see something different

Thanks for looking.

Choose a reason for hiding this comment

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

Hi Peter, the catches are removed, can it be re-reviewed, please?

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.

4 participants