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

Remove broken offline special-casing when retrieving cached data #1034

Merged
merged 7 commits into from Jan 14, 2019

Conversation

Projects
None yet
2 participants
@50Wliu
Copy link
Member

50Wliu commented Jan 3, 2018

Requirements

  • Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
  • All new code requires tests to ensure against regressions

Description of the Change

The fetchFromCache method looks up if there is cached data. Then, if it hasn't expired yet or Atom doesn't have internet access, it returns the cached data. Otherwise, if there was no cached data and Atom wasn't online, it used to return the empty object {} "so callers don't crash". Ironically, this very line causes the featured packages lookup to "crash" with an exception since it expects an array. Therefore in this PR I have removed the special-casing and always return null without internet access and let the callers deal with it (they already do).

Additionally I have simplified fetchFromCache by making it sync (there was nothing requiring it to accept a callback) and removing an unused options parameter.

Alternate Designs

I'm going to rename this "Future Aspirations". A getaddrinfo is much better than throwing an exception but I would still like to see a warning rather than an error; something like "Atom is currently offline and the following results may be outdated" or "Atom is currently offline. Try again later" if there's no results. That would require the implementation of a new WarningView however and is why I decided not to do that in this PR.

Benefits

No uncaught exceptions when loading the Install page for the first time without internet access.

Possible Drawbacks

Rather than generating an exception, we now generate a getaddrinfo ENOENT atom.io:443 error, but at least that's gracefully handled by settings-view.

Applicable Issues

Fixes #962
Going to say that this also fixes #668

50Wliu added some commits Jan 2, 2018

Temporarily remove broken offline special-casing
Certain callers expect the cached data to be an array, while others expect objects

For now, fail trying to hit atom.io instead of throwing an uncaught exception due to an unexpected data type.
@smashwilson

This comment has been minimized.

Copy link
Member

smashwilson commented Jan 14, 2019

👍 Reproduced the bug and verified the fix. Thanks!

@smashwilson smashwilson merged commit ee4fdee into master Jan 14, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@smashwilson smashwilson deleted the wl-fix-caching branch Jan 14, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.