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.

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

Use flutter.js bootstrapping #2792

Merged
merged 12 commits into from
May 9, 2024
Merged

Use flutter.js bootstrapping #2792

merged 12 commits into from
May 9, 2024

Conversation

johnpryan
Copy link
Contributor

@johnpryan johnpryan commented Jan 11, 2024 •
edited
Loading

This changes sketch_pad's bootstrapping to use flutter.js.

Fixes #2261

@johnpryan
Copy link
Contributor Author

This will depend on https://github.com/flutter/engine/pull/49782/files getting merged first, so leaving this as a draft for now.

@johnpryan johnpryan marked this pull request as ready for review February 15, 2024 22:43
@johnpryan johnpryan requested a review from devoncarew February 15, 2024 22:43
@johnpryan
Copy link
Contributor Author

cc: @ditman

@johnpryan johnpryan changed the title (Draft) Use flutter.js bootstrapping Use flutter.js bootstrapping Feb 15, 2024
@johnpryan
Copy link
Contributor Author

This changes some dart_services code, so you need to test by running dart_services (grind build-storage-artifacts and dart bin/server.dart and use?channel=localhost in the URL)

@johnpryan
Copy link
Contributor Author

@devoncarew I've tested this on the latest stable channel and everything is working smoothly. PTAL when you can.

ditman
ditman approved these changes Feb 15, 2024
View reviewed changes
Copy link
Member

@ditman ditman left a comment

Choose a reason for hiding this comment

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

There's a new, new flutter.js initialization coming, but that's not documented yet. This LGTM!

pkgs/sketch_pad/web/frame.js Outdated Show resolved Hide resolved
devoncarew
devoncarew approved these changes Feb 15, 2024
View reviewed changes
Copy link
Member

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

rslgtm for my part, though I'd rely on @ditman for the bulk of the review

@parlough
Copy link
Member

In general, looks great to me, but due to the dart-services changes is there a desync with clients that could cause them to not work temporarily if connecting to an older server? Same for my temporary use of the legacy frontend.

@johnpryan
Copy link
Contributor Author

@parlough you're right, let's hold off until we've full migrated the websites.

@johnpryan
Copy link
Contributor Author

@parlough have we migrated the websites?

@johnpryan johnpryan mentioned this pull request Mar 8, 2024
Closed
parlough
parlough approved these changes Apr 6, 2024
View reviewed changes
Copy link
Member

@parlough parlough left a comment

Choose a reason for hiding this comment

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

The websites are both using SketchPad now so feel free to land this when you're ready. Thanks for working on this and I appreciate the patience :)

@johnpryan
Copy link
Contributor Author

This is working for me after merging with main, so I'm going to land this and keep testing.

@johnpryan johnpryan merged commit 1c704c9 into dart-lang:main May 9, 2024
5 checks passed
@johnpryan johnpryan deleted the flutter-js branch May 9, 2024 18:01
kevmoo
kevmoo reviewed May 9, 2024
View reviewed changes
pkgs/dartpad_ui/web/frame.js
var url = URL.createObjectURL(blob);
_flutter.loader.loadEntrypoint({
entrypointUrl: url,
onEntrypointLoaded: async function(engineInitializer) {
Copy link
Member

Choose a reason for hiding this comment

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

I think you can even drop this bit. This is wiring up the default behavior I believe.

Right @ditman ?

johnpryan added a commit that referenced this pull request May 9, 2024
@johnpryan
Revert "Use flutter.js bootstrapping (#2792)"
ce0b1e5 
This reverts commit 1c704c9.
@johnpryan johnpryan mentioned this pull request May 9, 2024
Merged
@johnpryan
Copy link
Contributor Author

This is causing a CORS error, so I'm going to roll this back. #2959

@johnpryan
Copy link
Contributor Author 

Access to fetch at 'https://dartpad.dev/canvaskit/chromium/canvaskit.wasm' from origin 'null' has been blocked by CORS policy: No 'Access-Control-Allow-Origin' header is present on the requested resource. If an opaque response serves your needs, set the request's mode to 'no-cors' to fetch the resource with CORS disabled.

johnpryan added a commit that referenced this pull request May 9, 2024
@johnpryan
Revert "Use flutter.js bootstrapping (#2792)" (#2959)
9183b0b 
This reverts commit 1c704c9.
johnpryan added a commit that referenced this pull request May 9, 2024
@johnpryan
Revert "Revert "Use flutter.js bootstrapping (#2792)" (#2959)"
12d4f27 
This reverts commit 9183b0b.
@johnpryan johnpryan mentioned this pull request May 9, 2024
Merged
johnpryan added a commit that referenced this pull request May 9, 2024
@johnpryan
Use flutter.js bootstrapping (#2960)
578bd4e 
* Revert "Revert "Use flutter.js bootstrapping (#2792)" (#2959)"

This reverts commit 9183b0b.

* Loosen CORS header for canvaskit.wasm
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kevmoo kevmoo kevmoo left review comments

@parlough parlough parlough approved these changes

@devoncarew devoncarew devoncarew approved these changes

@ditman ditman ditman approved these changes

Assignees

@johnpryan johnpryan

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Use flutter.js API
5 participants
@johnpryan @parlough @kevmoo @ditman @devoncarew