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

[release/7.0] [browser] make dynamic import cancelable #80312

Merged

Conversation

pavelsavara
Copy link
Member

@pavelsavara pavelsavara commented Jan 6, 2023

Backport of #80257 to release/7.0

The underlying dynamic import() API of the browser is not cancelable.

During design of the C# API we added the cancelation token because we do that on all async methods.
We also added the C# code which would cancel/abandon the JS promise.
But we forgot to make the JS promise ready for that.
This PR fixes the omission.

It doesn't really cancel the JS download, nor execution of the module as that's not possible.
But it at least would stop blocking the caller.

Customer Impact

Fixes customer reported issue #80028

Testing

Unit test

Risk

Low, new API in Net7

@pavelsavara pavelsavara added this to the 7.0.x milestone Jan 6, 2023
@pavelsavara pavelsavara requested a review from maraf January 6, 2023 20:48
@pavelsavara pavelsavara self-assigned this Jan 6, 2023
@ghost
Copy link

ghost commented Jan 6, 2023

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

Issue Details

Backport of #80257 to release/7.0

Customer Impact

Fixes customer reported issue #80028

Testing

Unit test

Risk

Low, new API in Net7

Author: pavelsavara
Assignees: pavelsavara
Labels:

arch-wasm, area-System.Runtime.InteropServices.JavaScript

Milestone: 7.0.x

@pavelsavara pavelsavara added the Servicing-consider Issue for next servicing release review label Jan 6, 2023
@carlossanlop carlossanlop removed the Servicing-consider Issue for next servicing release review label Jan 9, 2023
@carlossanlop
Copy link
Member

Removing servicing-consider because the PR is still marked as draft. When ready, please re-add the label and send an email to Tactics requesting approval.

@pavelsavara
Copy link
Member Author

The CI fail is unrelated here and re-run didn't help.
Is that OK @carlossanlop ?
If not, what's my next step ?

@pavelsavara pavelsavara marked this pull request as ready for review January 11, 2023 18:17
@pavelsavara pavelsavara requested a review from lewing as a code owner January 11, 2023 18:17
@carlossanlop
Copy link
Member

carlossanlop commented Jan 11, 2023

@pavelsavara please make sure to fill out the template explaining how the issue affects customers and how the fix will address it. Once that's done, please send email to Tactics requesting merge approval,. and add the servicing-consider label. The Code Complete cut-off is on Friday 13th (less than 2 days from now), so we need to get this merged before EOD on Friday so that we have enough time to monitor the base branch in case anything breaks. Next weekend is a long weekend (Monday is a holiday in the US) so we are trying to avoid bottlenecks next week.

@carlossanlop
Copy link
Member

The CI failure in dev-innerloop is known and unrelated: #80284

@pavelsavara
Copy link
Member Author

Thanks, I updated the description.
It was already approved by email.

@pavelsavara pavelsavara added the Servicing-approved Approved for servicing release label Jan 12, 2023
@carlossanlop carlossanlop modified the milestones: 7.0.x, 7.0.3 Jan 12, 2023
@carlossanlop
Copy link
Member

Approved by Tactics via email (7.0.3).
Signed off by area owner.
CI failure unrelated.
No OOB changes needed (browser code).
Ready to merge. :shipit:

@carlossanlop carlossanlop merged commit 600c612 into dotnet:release/7.0 Jan 12, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Feb 11, 2023
@pavelsavara pavelsavara deleted the backport/pr-80257-to-release/7.0 branch September 2, 2024 15:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-wasm WebAssembly architecture area-System.Runtime.InteropServices.JavaScript Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants