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

[wasm] fix error cases in assets loading #73702

Merged
merged 10 commits into from
Aug 11, 2022
Merged

[wasm] fix error cases in assets loading #73702

merged 10 commits into from
Aug 11, 2022

Conversation

pavelsavara
Copy link
Member

@pavelsavara pavelsavara commented Aug 10, 2022 •
edited
Loading

  • fix bad rename of maxParallelDownloads - throttling
  • fix re-try logic
  • fixed counting of loaded assets
  • catching more error cases
  • fix blazor detection

Fixes #73673

@pavelsavara pavelsavara added this to the 7.0.0 milestone Aug 10, 2022
@pavelsavara pavelsavara requested a review from maraf August 10, 2022 17:03
@pavelsavara pavelsavara self-assigned this Aug 10, 2022
@ghost
Copy link

ghost commented Aug 10, 2022

Tagging subscribers to 'arch-wasm': @lewing
See info in area-owners.md if you want to be subscribed.

Issue Details
  • fix bad rename of maxParallelDownloads - fixes throttling
Author: pavelsavara
Assignees: pavelsavara
Labels:

arch-wasm, area-System.Runtime.InteropServices.JavaScript
Milestone: 7.0.0

@pavelsavara
Copy link
Member Author

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@pavelsavara pavelsavara changed the title Wasm fix optional assets [wasm] fix error cases in assets loading Aug 10, 2022
This was referenced Aug 10, 2022
Closed
Closed
@pavelsavara
Copy link
Member Author

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@pavelsavara pavelsavara marked this pull request as ready for review August 10, 2022 18:09
kg
kg reviewed Aug 11, 2022
View reviewed changes
src/mono/wasm/runtime/assets.ts Outdated Show resolved Hide resolved
radical
radical reviewed Aug 11, 2022
View reviewed changes
src/mono/wasm/runtime/assets.ts Show resolved Hide resolved
src/mono/wasm/runtime/assets.ts Show resolved Hide resolved
src/mono/wasm/runtime/assets.ts Outdated Show resolved Hide resolved
src/mono/wasm/runtime/assets.ts Outdated Show resolved Hide resolved
src/mono/wasm/runtime/assets.ts Outdated Show resolved Hide resolved
src/mono/wasm/runtime/assets.ts Outdated Show resolved Hide resolved
src/mono/wasm/runtime/assets.ts Outdated Show resolved Hide resolved
src/mono/wasm/runtime/assets.ts Show resolved Hide resolved
src/mono/wasm/runtime/assets.ts Outdated Show resolved Hide resolved
kg
kg reviewed Aug 11, 2022
View reviewed changes
src/mono/wasm/runtime/assets.ts Outdated Show resolved Hide resolved
kg
kg reviewed Aug 11, 2022
View reviewed changes
src/mono/wasm/runtime/assets.ts Outdated Show resolved Hide resolved
kg
kg reviewed Aug 11, 2022
View reviewed changes
src/mono/wasm/runtime/assets.ts Outdated Show resolved Hide resolved
radical
radical approved these changes Aug 11, 2022
View reviewed changes
Copy link
Member

@radical radical left a comment

Choose a reason for hiding this comment

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

I'm no expert on JS/TS, and reviewed whatever I could. Other than my comments, it looks good to me.

@pavelsavara
Copy link
Member Author

thanks @kg !

@pavelsavara
Copy link
Member Author

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

maraf
maraf reviewed Aug 11, 2022
View reviewed changes
src/mono/wasm/runtime/assets.ts Outdated Show resolved Hide resolved
src/mono/wasm/runtime/assets.ts Outdated Show resolved Hide resolved
@pavelsavara
Copy link
Member Author

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

thaystg
thaystg approved these changes Aug 11, 2022
View reviewed changes
Copy link
Member

@thaystg thaystg left a comment

Choose a reason for hiding this comment

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

LGTM. I also tested it on codespaces :)

@pavelsavara
Copy link
Member Author

pavelsavara commented Aug 11, 2022 •
edited
Loading

There was some crypto timeouts on this PR, but it went away on re-run.
All the other failures are #73721

@pavelsavara pavelsavara merged commit 478a8a2 into dotnet:main Aug 11, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Sep 11, 2022
@pavelsavara pavelsavara deleted the wasm_fix_optional_assets branch November 29, 2022 16:22
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@kg kg kg left review comments

@radical radical radical approved these changes

@maraf maraf maraf approved these changes

@thaystg thaystg thaystg approved these changes

@lewing lewing Awaiting requested review from lewing lewing is a code owner

Assignees

@pavelsavara pavelsavara

Labels
arch-wasm WebAssembly architecture area-System.Runtime.InteropServices.JavaScript
Projects
None yet
Milestone
7.0.0
Development

Successfully merging this pull request may close these issues.

[wasm] Regression: Crash on running debugger tests on Codespaces
5 participants
@pavelsavara @radical @kg @thaystg @maraf