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] [wasm] Catch error from loading "node:crypto" module #80249

Conversation

carlossanlop
Copy link
Member

Backport of #78916 to release/7.0

Important: This is a resubmission of the previous backport PR submitted by @maraf that I accidentally closed instead of squash+merged. I was unable to reopen the PR because I had deleted the branch. This PR has already been approved by Tactics.

Customer Impact

Based on node documentation, there might be a build of node without "node:crypto". In such a case the module import will throw an exception. In #78696 (backport #78766) we load "node:crypto" module during the runtime startup and in this PR we are catching such exception and rethrowing it only when user tries to use the API.

It restores the behavior before #78696 when crypto API is not available.
It should be included in the same release as #78696 (backport #78766).

Testing

Manual

Risk

None

IMPORTANT: Is this backport for a servicing release? If so and this change touches code that ships in a NuGet package, please make certain that you have added any necessary package authoring and gotten it explicitly reviewed.

* Catch error from loading node:crypto module.
* Throw error with explanation when crypto module is not available.
* Fix providing error throwing polyfill.
@carlossanlop carlossanlop added Servicing-approved Approved for servicing release arch-wasm WebAssembly architecture area-System.Runtime.InteropServices.JavaScript labels Jan 5, 2023
@carlossanlop carlossanlop added this to the 7.0.3 milestone Jan 5, 2023
@carlossanlop carlossanlop self-assigned this Jan 5, 2023
@ghost
Copy link

ghost commented Jan 5, 2023

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

Issue Details

Backport of #78916 to release/7.0

Important: This is a resubmission of the previous backport PR submitted by @maraf that I accidentally closed instead of squash+merged. I was unable to reopen the PR because I had deleted the branch. This PR has already been approved by Tactics.

Customer Impact

Based on node documentation, there might be a build of node without "node:crypto". In such a case the module import will throw an exception. In #78696 (backport #78766) we load "node:crypto" module during the runtime startup and in this PR we are catching such exception and rethrowing it only when user tries to use the API.

It restores the behavior before #78696 when crypto API is not available.
It should be included in the same release as #78696 (backport #78766).

Testing

Manual

Risk

None

IMPORTANT: Is this backport for a servicing release? If so and this change touches code that ships in a NuGet package, please make certain that you have added any necessary package authoring and gotten it explicitly reviewed.

Author: carlossanlop
Assignees: carlossanlop, maraf
Labels:

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

Milestone: 7.0.3

@carlossanlop
Copy link
Member Author

Approved by Tactics (7.0.3).
Signed off by area owners.
No OOB changes needed.
The CI failure seems to be a sudden crash of the test process.
Ready to merge. :shipit:

@carlossanlop carlossanlop merged commit 7a88642 into dotnet:release/7.0 Jan 5, 2023
@carlossanlop carlossanlop deleted the backport/pr-78696-to-release/7.0 branch January 5, 2023 22:51
@ghost ghost locked as resolved and limited conversation to collaborators Feb 5, 2023
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.

4 participants