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

Switch to blazor in the client #3254

Closed
5 of 6 tasks
pbiggar opened this issue Nov 25, 2021 · 22 comments
Closed
5 of 6 tasks

Switch to blazor in the client #3254

pbiggar opened this issue Nov 25, 2021 · 22 comments
Assignees

Comments

@pbiggar
Copy link
Member

pbiggar commented Nov 25, 2021

  • Get blazor working on darklang.com
  • switch integration tests over to using blazor
  • ensure when blazor fails that we get a rollbar
  • add changelog entry explaining how to change back
  • Go through user canvases and check Blazor works properly - @pbiggar only
  • switch the client's default to blazor
@StachuDotNet
Copy link
Member

StachuDotNet commented Jan 31, 2022

I've found the use-blazor query param, and have found that basic functionality seems OK with that flag on.

Could you please remind me the issues that I can address? @pbiggar

Many in the TODO list here seem only actionable by you, besides the rollbar one which I'll think/look through. I recall that there were some perf. concerns in particular, but forget where.
(others I can do, but only once we're OK to commit to the switch-over)

@pbiggar
Copy link
Member Author

pbiggar commented Jan 31, 2022

I've reordered them and marked the one that I think only I can do.

To switch integration tests, you would add the setting here.

(separately, I just realized integration tests are running on port 8000 by default, we should be safe to switch the default (see --fsharp in run.sh)).

The issue that I expect to come up is that blazor is so slow to initialize that the integration tests will be hard to make work. That slowness is a real issue, and potentially blocking. The issue to solve it is #3168

The changelog is in the docs repo, maybe make a PR saying that it will happen and we can update when we switch the default?

Btw, feel free to make new issues for any of these if you prefer.

@StachuDotNet
Copy link
Member

relevant bug: #3486

@StachuDotNet
Copy link
Member

StachuDotNet commented Feb 21, 2022

Small check-in here; working on this in #3485. Basic transition is set up there, and remaining work at this point is:

  • resolve a small group of integration tests
  • punt some others with FSTODOs
  • ensure when blazor fails that we get a rollbar
  • add changelog entry

After that point, I'll have to hand off to Paul to test with various user canvases.

@StachuDotNet
Copy link
Member

Quick link for benchmarking: https://benchmarkdotnet.org/articles/samples/IntroWasm.html

@StachuDotNet
Copy link
Member

StachuDotNet commented Mar 16, 2022

Catching up with some Slack threads, the remaining things to do here are:

  • cancel analysis executions when new requests come in, to decrease UX pains of non-AOT Blazor being slow.
    This is my current focus
  • reproduce and resolve the loading issue that Paul faced. I haven't been able to repro - @pbiggar if you could share your experience in more detail, that'd be appreciated.
  • resolve a (separate?) load-time issue that I faced[1]

[1] I managed to hit a 404 when loading blazor.boot.js, when using F# ApiServer in prod (using a-testing-fsharp and fsharp-backend=true). I believe the problem here is involved with some of:

Relevantly, the OCaml equivalent not doesn't exclude the blazor folder from things it expects to be hashed

I'm guessing the thing to do here will be to either change _push-assets-to-cdn to not hash those files, or change the F# solution to expect them to be hashed. Thoughts, @pbiggar? Intuitively, the second path here makes more sense to me.

@pbiggar
Copy link
Member Author

pbiggar commented Mar 16, 2022

I think I must have been confused when I was adding those files. I think hashing them is the way to go, and what we have working in OCaml is probably right.

@pbiggar
Copy link
Member Author

pbiggar commented Mar 16, 2022

I managed to reproduce the error:

Uncaught exception Blazor worker failure TypeError: Failed to fetch
    at loadResource (blob:https://darklang.com/dacea6f8-981c-4a6a-9522-f78faeed6aeb:738:24)
    at blob:https://darklang.com/dacea6f8-981c-4a6a-9522-f78faeed6aeb:732:11
    at Array.map (<anonymous>)
    at Object.loadResources (blob:https://darklang.com/dacea6f8-981c-4a6a-9522-f78faeed6aeb:731:39)
    at createEmscriptenModuleInstance (blob:https://darklang.com/dacea6f8-981c-4a6a-9522-f78faeed6aeb:220:52)
    at blob:https://darklang.com/dacea6f8-981c-4a6a-9522-f78faeed6aeb:772:25 undefined undefined undefined
e.<computed> @ appsupport-0aab008024f907c251bfe9927856eeebed075655f5139be469f7af1ceaf66459.js:179373

image

And it seems to happen when part of the load time's out:

image

Unclear why these loads time out, but this happens in about 1/4 of the times I refresh.

@StachuDotNet
Copy link
Member

I continue to be unable to repro the blazor load problems.
Could you take that one, @pbiggar?

I've tried multiple machines, with and without network throttling, with and without network caching.

@pbiggar
Copy link
Member Author

pbiggar commented Mar 17, 2022

Just the load issues from this comment, right? #3254 (comment)

@StachuDotNet
Copy link
Member

Yes. I'm continuing to work on the file hashing issue and cancelling analysis requests when new ones come in.

@StachuDotNet
Copy link
Member

Amazing. Suddenly I can repro on my laptop. That said, getting a bit burnt on Blazor work, so just going to focus on the other 2 tasks for now.

@pbiggar
Copy link
Member Author

pbiggar commented Mar 17, 2022

OK, so you're taking #3589, or did you want to drop it for now?

@StachuDotNet
Copy link
Member

I'll drop it for now. Just funny timing.

@StachuDotNet
Copy link
Member

Relates to #3642

@StachuDotNet
Copy link
Member

@pbiggar - your idea to create an absolutely-minimal AOT build, get that working (callable from JS), and un-delete things slowly until we find the breaking point hadn't occurred to me before. I'm happy to pursue that solo after my current task (or later), or we can pair on it like you mentioned. Mostly just recording the idea here as a reminder.

@pbiggar
Copy link
Member Author

pbiggar commented May 8, 2022

I'll be unavailable except sporadicly the next week, so better to go alone. Ping me if you'd like to discuss though, I'll have some availability.

@pbiggar
Copy link
Member Author

pbiggar commented May 8, 2022

I'd suggest that you use the official Blazor js files, not our custom ones

@StachuDotNet
Copy link
Member

I'd suggest that you use the official Blazor js files, not our custom ones

Hmm, I'm not sure how. Are your referring to BlazorWorker? I thought we had to adjust/merge a bunch of files to make it work for our (non-UI) use case?

@pbiggar
Copy link
Member Author

pbiggar commented May 8, 2022

Hmm, I'm not sure how. Are your referring to BlazorWorker? I thought we had to adjust/merge a bunch of files to make it work for our (non-UI) use case?

Yeah, but for bug-hunting that doesn't matter. We don't need to ship the outcome, just find where things break.

I'd suggest editing ui.html to use their blazorworker.js instead of ours, and it's not really important if it's in the background or not for now. So when a trace comes in, it would run it in the foreground instead I guess.

@StachuDotNet
Copy link
Member

OK I'll have to look into that and follow up.

Is that suggestion based on the chance that our BlazorWorker is somehow the issue causing AOT builds to fail?

@StachuDotNet
Copy link
Member

Hopefully merging this later today.

  • remove &use-blazor=true from integration-tests/tests.ts
    (will be added in AOT switch-over PR)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

No branches or pull requests

2 participants