-
Notifications
You must be signed in to change notification settings - Fork 10.1k
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
Support for custom boot resource loading (e.g., for CDNs) #20903
Conversation
Assert.Empty(subsequentResourcesRequested.Where(path => path.EndsWith(".dll"))); | ||
} | ||
|
||
[Fact(Skip = "https://github.com/dotnet/aspnetcore/issues/20154")] | ||
[Fact] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't the fact that this has failed in the past enough evidence that it's flaky? That's somewhat the bar we use. If it fails it's quarantined/flaky and then we either do a fix or add more diagnostics before unflakying it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't the fact that this has failed in the past enough evidence that it's flaky?
It's a question of degree. If this had failed often then yes, but a single failure would point more to environmental issues than test issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even if it's 99.99% solid we need to look at it, as with the amount of tests that we run daily it will cause significant build failures.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The approach to reliability for E2E tests is a separate question from this PR so we could have a separate discussion about that. Spoiler: I don't believe expecting 100% reliability for browser automation is realistic, so a retry-based approach makes sense. Through retries, we can get arbitrarily close to 100%.
Regarding this specific test, I'm happy to go either way on it. If anyone feels strongly I'll put back the Skip
marker. If nobody speaks up I'll leave it un-skipped since there isn't clear evidence from the implementation that it's different from the other tests. So, shout up if you want 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The CachesResourcesAfterFirstLoad
test failed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was a real failure due to the change here - fix implemented.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good!
…atching newer type definitions.
219332a
to
52afc53
Compare
This PR makes it possible to customize how Blazor WebAssembly's boot resources are loaded. The primary use case would be to load certain resources from an external CDN, but you could also use it if for some reason you needed to modify how outbound requests are constructed.
Usage example
In
index.html
:The complete API definition is at https://github.com/dotnet/aspnetcore/compare/blazor-wasm...stevesa/cdn?expand=1#diff-7213cd49bb3e575d741b86264246999f
Developers don't have to supply behavior for all resource types. For CDN scenarios, you'd only want to load from CDN the resources that aren't custom to your application. That would include the
dotnet.*.js
,dotnet.wasm
, and timezone data files, as shown in the above example.The
loadBootResource
function can return a URL string (which is then fetched using the framework's default options, including integrity checking), or aPromise<Response>
. For example, you canreturn fetch(someUrl);
and that way you could set custom HTTP headers or whatever.Of course, external sources need to return the required CORS headers for browsers to allow this. But CDNs already know about that.
Other code review notes
I recommend reviewing this on a commit-by-commit basis so you can most easily decouple the TypeScript changes from each other.
Reason: as part of this, I was getting some interference from TypeScript because we were using a pretty old version of the TypeScript compiler, which doesn't match up with the VS Code tooling.
So, I upgraded the
typescript
package and for good measure also upgraded our other NPM dependencies. This wasn't strictly required, but this is our last chance to get them up-to-date while still having a couple of preview releases ahead in case it causes any surprises. It will be better to ship the final version with relatively fresh dependencies since this version needs to remain good for a long time.As a further consequence of this, I had to update to more up-to-date versions of some Emscripten APIs in
MonoPlatform.ts
. This is also a good thing because we're no longer relying on obsolete APIs there now. Those are some really low-level APIs which really have to be perfect and perform well, but I don't think there's any significant risk as the changes are quite straightforward and mechanical.