Skip to content

Add offline mode #2483

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Mar 21, 2025
Merged

Add offline mode #2483

merged 2 commits into from
Mar 21, 2025

Conversation

RossComputerGuy
Copy link
Contributor

  • Adds an --offline flag which is useful for sandbox/networkless build systems such as Nix.
  • I’ve reviewed the contributor guide and applied the relevant portions to this PR.
Contribution guidelines:

Note that many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.

@bkonyi
Copy link
Collaborator

bkonyi commented Oct 7, 2024

Sorry for the delay on this @RossComputerGuy! Overall, this change looks good to me, but we should add some tests before landing.

@bkonyi bkonyi self-requested a review October 7, 2024 16:35
@RossComputerGuy
Copy link
Contributor Author

I've gone ahead and rebased it. I will look into getting tests in shortly.

@RossComputerGuy
Copy link
Contributor Author

Can't figure out how the test should be done, I see there's configuration tests but none invoke read.

@bkonyi
Copy link
Collaborator

bkonyi commented Oct 8, 2024

Can't figure out how the test should be done, I see there's configuration tests but none invoke read.

I think you can either add a test like this or something similar to what's done in this file.

Basically, you'll just want a test that runs webdev serve --no-pub on a project that hasn't had dart pub get invoked to pull down the dependencies. The webdev serve --no-pub should fail to serve the project since there will be unresolved dependencies.

@RossComputerGuy
Copy link
Contributor Author

Ok, I added the test.

bkonyi
bkonyi reviewed Oct 8, 2024
View reviewed changes
webdev/test/installation_test.dart
@@ -30,6 +30,14 @@ void main() {
Process? serveProcess;
Directory? tempDir0;

final testScript =
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not really sure what's going on with these lines. Could we not get the package directory simply by doing p.dirname(p.dirname(Platform.script.toFilePath())?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That gives me /tmp

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah right, because the test runner generates an entrypoint...

In that case, I think we should continue to activate webdev from pub instead of using --source path. This test should still work as long as we don't run dart pub get on the created project before running webdev serve --offline.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh ok, I was assuming we would do it the way I did so new updates would actually work. I was getting an error saying --offline wasn't a recognized flag even if I did activate it outside the test.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Right, that's a great point. Given that, I think we should move the test to webdev/test/integration_test.dart. The tests there run webdev from source so we won't have to worry about publishing before this new test will pass.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tbf, I could see how it could fit into both. Especially since once the next update is published and this PR is in that, then the flag would be available.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've got the test added to the integration set.

@RossComputerGuy RossComputerGuy force-pushed the fix/nix-build branch 2 times, most recently from 418f680 to 6fbec3a Compare October 8, 2024 21:10
@bkonyi bkonyi requested a review from jyameo March 13, 2025 18:07
@bkonyi
Copy link
Collaborator

bkonyi commented Mar 13, 2025

Sorry for the delay on this @RossComputerGuy. Can you take this over the line @jyameo?

jyameo
jyameo approved these changes Mar 19, 2025
View reviewed changes
Copy link
Contributor

@jyameo jyameo left a comment

Choose a reason for hiding this comment

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

Thanks so much for your contribution! Apologies for the delay in getting this over the line. Overall looks good! We really appreciate your work on this!

@jyameo
Copy link
Contributor

jyameo commented Mar 20, 2025

@RossComputerGuy Can you please address the CI failures below and include an entry in changelog.md detailing this change?

@RossComputerGuy
Copy link
Contributor Author

I was able to address everything except the dart analyze because the two annotations don't like being used together apparently.

@jyameo
Copy link
Contributor

jyameo commented Mar 20, 2025 •
edited
Loading

I was able to address everything except the dart analyze because the two annotations don't like being used together apparently.
I will take a look at the analyzer issue. For the changelog, it seems you a few entries as we are currently at #3.7.2-wip. Can you make sure to pull the latest main branch and merge it into yours

@RossComputerGuy RossComputerGuy force-pushed the fix/nix-build branch 2 times, most recently from 15dfec8 to c8dae6f Compare March 20, 2025 18:53
@jyameo
Copy link
Contributor

jyameo commented Mar 20, 2025

I was able to address everything except the dart analyze because the two annotations don't like being used together apparently.

You should be able to resolve thedart analyze issue by applying the library directive like this:

@Timeout(Duration(minutes: 3))
library;

Similar to what's being done in the installation_test.dart

@RossComputerGuy
Copy link
Contributor Author

Oh, I was looking at the wrong test.

@jyameo
Copy link
Contributor

jyameo commented Mar 21, 2025 •
edited
Loading

Thanks for the update @RossComputerGuy. To finalize this PR, can you please address the following issue:

  • Create an entry in the webdev README describing the use of --offline.
  • Add a new section at the top of the DWDS CHANGELOG for ##24.3.8-wip (no need to enter any details, just add the section header).
  • Update the version of DWDS to 24.3.8-wip in the pubspec file.
  • Update the package version to 24.3.8-wip in the version.dart file.

Also please pull and merge the latest version of the main branch before making the changes above.

Once these are addressed, we should be good to go.

@RossComputerGuy
Copy link
Contributor Author

To finalize this PR, can you please address the following issue

Done all those now.

@jyameo
Copy link
Contributor

jyameo commented Mar 21, 2025

To finalize this PR, can you please address the following issue

Done all those now.

Awesome. We just need to pull down and merge the latest version of the main branch and resolve the conflicts and we should be good

@RossComputerGuy
Copy link
Contributor Author

Done

@jyameo
Copy link
Contributor

jyameo commented Mar 21, 2025 •
edited
Loading

Done

Apologies @RossComputerGuy. I made a mistake... the version the DWDS changelog, pubspec, and version.dart should actually be 24.3.9-wip instead of 24.3.8-wip. Can you please rectify this. Sorry for the confusion.

@RossComputerGuy
Copy link
Contributor Author

Done

@jyameo
Copy link
Contributor

jyameo commented Mar 21, 2025

Hey @RossComputerGuy, for some reason the test/readme_test.dart is still failing. Can you please look into this. The other failures (ie. e2e_test.dart) can be ignored as it was already failing. Thanks for the patience and collaboration

@jyameo
Copy link
Contributor

jyameo commented Mar 21, 2025

Hey @RossComputerGuy, for some reason the test/readme_test.dart is still failing. Can you please look into this. The other failures (ie. e2e_test.dart) can be ignored as it was already failing. Thanks for the patience and collaboration

Looks like it's caused by some white whitespace issues. the Readme needs to match exactly the output of the command webdev build/serve.

jyameo
jyameo reviewed Mar 21, 2025
View reviewed changes
jyameo
jyameo reviewed Mar 21, 2025
View reviewed changes
@jyameo jyameo merged commit af33df7 into dart-lang:main Mar 21, 2025
43 of 47 checks passed
@RossComputerGuy RossComputerGuy deleted the fix/nix-build branch March 21, 2025 19:33
@jyameo
Copy link
Contributor

jyameo commented Mar 21, 2025

Thanks @RossComputerGuy for your contribution and patience in landing this. Your changes are in and will be part of the next release.

copybara-service bot pushed a commit to dart-lang/sdk that referenced this pull request Apr 2, 2025
@devoncarew
[deps] rev core, dartdoc, ecosystem, http, i18n, shelf, test, webdev
d56749c 
Revisions updated by `dart tools/rev_sdk_deps.dart`.

core (https://github.com/dart-lang/core/compare/61e6771..a6e81e0):
  a6e81e0b  2025-03-31  Lasse R.H. Nielsen  Make URL strategy better at recognizing URLs. (dart-lang/core#873)
  379e9c2e  2025-03-25  dependabot[bot]  Bump dart-lang/setup-dart from 1.7.0 to 1.7.1 in the github-actions group (dart-lang/core#869)

dartdoc (https://github.com/dart-lang/dartdoc/compare/d400676..cbc1885):
  cbc18857  2025-03-25  Sam Rawlins  Improve the error message when an import cannot be resolved (dart-lang/dartdoc#4023)
  18a7631d  2025-03-25  Parker Lougheed  Place footer below main content (dart-lang/dartdoc#4014)
  f41bd3ab  2025-03-24  Sam Rawlins  Improve failure when import cannot be found (dart-lang/dartdoc#4022)

ecosystem (https://github.com/dart-lang/ecosystem/compare/2317263..391a80c):
  391a80c  2025-04-01  Brian Wilkerson  Update prompts.dart (dart-lang/ecosystem#349)
  6a89899  2025-04-01  Brian Wilkerson  Update create_tuning_data.dart (dart-lang/ecosystem#350)

http (https://github.com/dart-lang/http/compare/1b6e28d..e988925):
  e988925  2025-03-31  Brian Quinlan  Remove .gitignored files (dart-lang/http#1728)
  29c5733  2025-03-25  Brian Quinlan  Improve the reliability of cronet_http/ok_http workflows (dart-lang/http#1736)
  dd56b86  2025-03-21  Brian Quinlan  Remove cronet_http.impl (dart-lang/http#1734)
  7f5ad18  2025-03-21  Brian Quinlan  Adding missing license headers (dart-lang/http#1735)
  fedead7  2025-03-20  Brian Quinlan  [cronet_http] Prepare to release 1.3.3 (dart-lang/http#1733)

i18n (https://github.com/dart-lang/i18n/compare/d9cce0b..2701040):
  2701040  2025-04-01  dependabot[bot]  Bump the github-actions group with 2 updates (dart-lang/i18n#959)
  5101533  2025-04-01  Moritz  Fix hashes.dart formatting (dart-lang/i18n#960)

shelf (https://github.com/dart-lang/shelf/compare/2af8529..082d3ac):
  082d3ac  2025-04-01  dependabot[bot]  Bump actions/cache from 4.2.2 to 4.2.3 in the github-actions group (dart-lang/shelf#475)

test (https://github.com/dart-lang/test/compare/9e349d0..c1fa1e6):
  c1fa1e69  2025-04-01  dependabot[bot]  Bump the github-actions group with 3 updates (dart-lang/test#2476)
  31a2d64c  2025-03-31  Nate Bosch  Sort reporter options (dart-lang/test#2475)

webdev (https://github.com/dart-lang/webdev/compare/302b6db..697f2f7):
  697f2f7f  2025-03-24  Jessy Yameogo  renamed DWDS injector parameter (dart-lang/webdev#2602)
  af33df71  2025-03-21  Tristan Ross  Add offline mode (dart-lang/webdev#2483)
  5c805f14  2025-03-21  Jessy Yameogo  Updated DWDS to include a boolean flag that enables debugging support when set to true (dart-lang/webdev#2601)

Change-Id: I7af88306b77efa3dab2bdef0d7543663317448f1
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/419682
Auto-Submit: Devon Carew <[email protected]>
Reviewed-by: Konstantin Shcheglov <[email protected]>
Commit-Queue: Konstantin Shcheglov <[email protected]>
@jyameo jyameo mentioned this pull request Apr 4, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jyameo jyameo jyameo approved these changes

@bkonyi bkonyi bkonyi left review comments

Assignees
No one assigned
Labels
package:dwds package:webdev
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@RossComputerGuy @bkonyi @jyameo