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

[static_shock_cli]: Handle SocketException when checking for Pub version (resolves #128) #131

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

Conversation

suragch
Copy link
Collaborator

@suragch suragch commented May 30, 2024

Handle SocketException when checking for Pub version.

static_shock_cli checks for a newer version of the package on pub.dev when running shock serve. It assumes that the user has an active internet connection, but this is not always the case.

While making a GET request when there is no internet connect, the http library defines and throws a ClientException. At a lower level, this is a dart:io SocketException. In order to avoid importing http, this PR just handles SocketException.

When a SocketException occurs, the code cancels the Pub check and logs a warning. This allows static_shock_cli to continue serving the site using the current local version. The alternative would be to abort the serve and tell the user to restore the internet connection. However, that would make for a bad user experience.

Issues to consider

  • This PR does not include a test. I didn't see any related tests for this type of functionality, and I'm uncertain how I would mock StaticShockCliVersion to test it.
  • UpgradeCommand and VersionCommand also check for Pub version updates. I didn't update the code there since that is outside of the scope of issue [Shock][CLI] - Support offline mode #128. However, at least VersionCommand should also handle socket exceptions.

@matthew-carroll
Copy link
Contributor

This issue probably deserves a more holistic approach. We have multiple plugins that talk to the network, too, such as the GitHub plugin. We probably need a concept of an online vs offline mode. I would imagine that trying to run a standard build without internet should fail, but a special offline mode could be requested via arguments. I believe Flutter works that way.

Feel free to post thoughts about where we might inject this control.

@suragch
Copy link
Collaborator Author

suragch commented Jun 1, 2024

@matthew-carroll

In the static_shock_cli package, I would say the following commands should work in offline mode:

  • version
  • serve
  • create

And these should fail if there is no internet:

  • build
  • upgrade

I'm not currently sure what an online/offline mode control would look like that spans both static_shock_cli and static_shock, but here are some comments:

  • Doesn't serving the site also build it? If running build in offline mode fails, we wouldn't want that to cause serve to fail.
  • Plugins like the GitHub plugin could individually decide their behavior based on the on/offline status. For example, I think the GitHub plugin should still work in offline mode. It would just return an empty list of contributors.

@matthew-carroll
Copy link
Contributor

Doesn't serving the site also build it? If running build in offline mode fails, we wouldn't want that to cause serve to fail.

Why not? If build fails I would expect serve to fail. What's the use case where you'd get meaningful value from serve without building the website?

For example, I think the GitHub plugin should still work in offline mode. It would just return an empty list of contributors.

I'm not sure that would be a desirable behavior in general. When build runs, it deletes the existing build first. So if you happen to run without network access and you didn't realize it, you just deleted the version of your website that had the list of contributors.

Can you review the Flutter CLI and see how much/little works offline without explicitly telling the CLI to go into offline mode?

@suragch
Copy link
Collaborator Author

suragch commented Jun 3, 2024

Why not? If build fails I would expect serve to fail. What's the use case where you'd get meaningful value from serve without building the website?

I'd like shock serve to work in offline mode even though shock serve internally builds the app on every update. That's fine for shock build to fail when building the final production version. I'm trying to differentiate shock build and whatever the internal build mechanism is, which I assume shock serve and shock build both share. Whatever build mechanism shock serve uses should not fail in offline mode just because it's offline.

I'm not sure that would be a desirable behavior in general. When build runs, it deletes the existing build first. So if you happen to run without network access and you didn't realize it, you just deleted the version of your website that had the list of contributors.

Good point. However, this is still shock serve not shock build that I'm talking about. Serving a slightly inferior version of the website during development is still better than not being able to serve anything at all. For me it's not that often that I'm without internet, but I have a friend in rural India who is regularly without internet.

Can you review the Flutter CLI and see how much/little works offline without explicitly telling the CLI to go into offline mode?

These Flutter CLI commands work while offline:

flutter --version
flutter help
flutter channel
flutter config --enable-windows-desktop
flutter analyze
flutter clean
flutter devices
flutter logs
flutter screenshot

flutter doctor works adequately well (note the network error messages):

Doctor summary (to see all details, run flutter doctor -v):
[✓] Flutter (Channel stable, 3.22.1, on macOS 13.6 22G120 darwin-x64, locale en)
[✓] Android toolchain - develop for Android devices (Android SDK version 34.0.0)
[✓] Xcode - develop for iOS and macOS (Xcode 15.0.1)
[✓] Chrome - develop for the web
[✓] Android Studio (version 2022.2)
[✓] IntelliJ IDEA Community Edition (version 2022.2.1)
[✓] VS Code (version 1.89.1)
[✓] Connected device (2 available)
    ! Error: Browsing on the local area network for iPhone SE (2nd generation). Ensure the device is unlocked and attached with a
      cable or associated with the same local area network as this Mac.
      The device must be opted into Developer Mode to connect wirelessly. (code -27)
[!] Network resources
    ✗ A network error occurred while checking "https://pub.dev/": Failed host lookup: 'pub.dev'
    ✗ A network error occurred while checking "https://storage.googleapis.com/": Failed host lookup: 'storage.googleapis.com'
    ✗ A network error occurred while checking "https://maven.google.com/": Failed host lookup: 'maven.google.com'
    ✗ A network error occurred while checking "https://cocoapods.org/": Failed host lookup: 'cocoapods.org'
    ✗ A network error occurred while checking "https://github.com/": Failed host lookup: 'github.com'

If you do explicitly allow offline mode (or flutter pub get is already up to date), these also work:

flutter create myproject --offline 
flutter run
flutter test

These CLI commands don't work:

flutter create
flutter upgrade
flutter build
flutter pub get

@matthew-carroll
Copy link
Contributor

matthew-carroll commented Jun 4, 2024

Looks like Flutter can provide local status information without being online.

For Static Shock that's just shock version.

I wasn't completely clear on your last two categories. You mentioned flutter create in both groups. Also, you said the first group was "explicitly allow offline" but only the first example passed --offline, so I wasn't sure whether each of those commands needed the --offline flag or not.

Assuming the following can't be done offline for Flutter...

flutter create
flutter upgrade
flutter build
flutter pub get

That would probably map to...

shock create
shock template
shock build

Then, finally, flutter run is probably analogous to shock serve.

Action Items:

  • I think shock create and shock template should be runnable even in offline mode, but when there's no internet, they shouldn't call pub get or shock build automatically. Those are both currently called at the end of the process automatically. We should guard those behaviors on connectivity.
  • For serve, until we figure out where the actual pipeline code fits into this, the part where serve runs a build should be gated on connectivity, similar to create and template.
  • shock upgrade should check for connectivity and then display an error if not connected to the network.
  • We still need to figure out whether we really want every pipeline step to have to check connectivity, itself, or whether the overall pipeline should have a concept of online vs offline mode, and then make it easy for pipeline steps to coordinate with that.

Even though shock create, shock template, and shock serve will have parts that work without the network, they should all provide reasonable warning output to make it clear what's wrong.

@suragch
Copy link
Collaborator Author

suragch commented Jun 4, 2024

I wasn't completely clear on your last two categories.

Yes, I wasn't very clear with those. The assumptions you made, though, were correct.

Additional clarification:

  • flutter create needs the --offline flag or it won't work without connectivity.
  • The -offline flag with flutter create has the following docs: "When "flutter pub get" is run by the create command, this indicates whether to run it in offline mode or not. In offline mode, it will need to have all dependencies already available in the pub cache to succeed."
  • There is no --offline flag for flutter run or flutter test. They work as long as the required dependencies are available, either through creating a project with the --offline flag or by running flutter pub get while the internet connection was previously available.
  • There is a --no-pub flag for both flutter run and flutter test. This flag turns off the flutter pub get when executing the command. I didn't experiment with this, though.

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.

2 participants