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

Re-host the dart web UI on v3 of the server protocol #2691

Merged
merged 12 commits into from
Nov 17, 2023

Conversation

devoncarew
Copy link
Member

@devoncarew devoncarew commented Oct 23, 2023

  • re-host the dart web UI on v3 of the server protocol

  • 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.

@devoncarew devoncarew marked this pull request as draft October 23, 2023 18:16
@devoncarew devoncarew changed the title re-host the dart web UI v3 of the server protocol Re-host the dart web UI on v3 of the server protocol Oct 23, 2023
@devoncarew
Copy link
Member Author

@johnpryan - no hurry on this; I'll likely have some questions about the workshops UI and testing mode code before landing this.

@devoncarew devoncarew marked this pull request as ready for review October 24, 2023 22:57
Copy link
Contributor

@johnpryan johnpryan left a comment

Choose a reason for hiding this comment

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

It would be good to keep the diagnostic messages and the test code features working for now. Is that something you can do in this PR?

pkgs/dart_pad/lib/embed.dart Outdated Show resolved Hide resolved
pkgs/dart_pad/lib/embed.dart Outdated Show resolved Hide resolved
Copy link
Member Author

@devoncarew devoncarew left a comment

Choose a reason for hiding this comment

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

Updated! What's a good way to test the codelabs locally?

pkgs/dart_pad/lib/embed.dart Outdated Show resolved Hide resolved
pkgs/dart_pad/lib/embed.dart Outdated Show resolved Hide resolved
@johnpryan
Copy link
Contributor

You can test the codelabs and workshops by loading the right URL. For example:

To test a workshop you can click on one of the links in the README and copy-paste the URL after the /. For example: localhost:8080/workshops.html?webserver=https://dartpad-workshops-io2021.web.app/inherited_widget

To test a codelab you can right-click and grab the src attribute on the embedded DartPad iframe from the codelab such as the Basic Flutter layout concepts codelab, and do the same thing. For example http://localhost:8080/embed-flutter.html?theme=dark&run=false&split=60&ga_id=false

@johnpryan
Copy link
Contributor

You can also verify the embeds are working using the example at http://localhost:8080/example/embed.html

@devoncarew
Copy link
Member Author

It would be good to keep the diagnostic messages and the test code features working for now. Is that something you can do in this PR?

I can definitely add back in the test code features - I'd assumed from reading through the code that it wasn't used, but was mistaken.

Do you think the diagnostic messages functionality is important? It's not currently exposed in the new UI, and I wasn't really planning to do it. I think it's useful for more sophisticated diagnostics in an IDE, but I don't think critical here.

@johnpryan
Copy link
Contributor

Do you think the diagnostic messages functionality is important?

IIRC this was to enable corrections and documentation links: #1676

@devoncarew
Copy link
Member Author

devoncarew commented Nov 14, 2023

I restored the contextMessages info and related UI; this PR still needs some manual testing through the codelabs and embeds.

@johnpryan
Copy link
Contributor

this PR still needs some manual testing through the codelabs and embeds.

I'm seeing an issue with all three versions where the analysis message is always the same:

Screenshot 2023-11-15 at 8 53 13 AM

@devoncarew
Copy link
Member Author

I'm seeing an issue with all three versions where the analysis message is always the same:

The protocol changed a bit in this PR (due to the inclusion context messages and changing how we stored location info on analysis issues); you'll need to run this against a local backend to see results.

That's a good reminder though; we may want to land the dart-services + sketchpad portion of this PR first, and follow up ~an hour later with the part moving the current UI over to the new backend.

@devoncarew
Copy link
Member Author

Codelabs and the tests UI both seem to be working (responding to the server, showing analysis issues, supporting run, ...).

@johnpryan
Copy link
Contributor

We might want to update CONTRIBUTING.md and other readme files to remove any references to protoc

@devoncarew
Copy link
Member Author

We might want to update CONTRIBUTING.md and other readme files to remove any references to protoc

👍 ; I'd like to do that in a follow-up (this PR has accumulated a bunch of changes).

I'll hold this PR until we determine the build issue with the master channel (we'll need that server to support the new protocol).

@johnpryan
Copy link
Contributor

johnpryan commented Nov 16, 2023

I'll hold this PR until we determine the build issue with the master channel (we'll need that server to support the new protocol).

The checks on the main branch of the repo are passing, it's only the Cloud Run deploy action for the Flutter main channel that's failing so this should be okay to merge.

@devoncarew
Copy link
Member Author

The checks on the main branch of the repo are passing, it's only the Cloud Run deploy action for the Flutter main channel that's failing so this should be okay to merge.

I think the checks were unintentionally all run against stable (which passed happily). The regression on main has now made its way to beta.

@devoncarew devoncarew merged commit d8194c1 into main Nov 17, 2023
5 checks passed
@devoncarew devoncarew deleted the dart_web_ui_use_v3 branch November 17, 2023 23:03
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.

None yet

2 participants