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

WebAssembly: Add PNSE for System.Net.Mail #38207

Merged
merged 1 commit into from
Jul 3, 2020

Conversation

akoeplinger
Copy link
Member

@akoeplinger akoeplinger commented Jun 22, 2020

It's not supported on WebAssembly so throw PlatformNotSupportedException.

@ghost
Copy link

ghost commented Jun 22, 2020

Tagging subscribers to this area: @dotnet/ncl
Notify danmosemsft if you want to be subscribed.

@akoeplinger akoeplinger requested a review from stephentoub June 22, 2020 13:47
@stephentoub
Copy link
Member

stephentoub commented Jun 22, 2020

It allows using the code paths that write a mail message to the file system

Will anyone actually use this in the browser? I'm wondering if we should instead just add something along the lines of:

<GeneratePlatformNotSupportedAssemblyMessage Condition="'$(TargetsBrowser)' == 'true'">SR.SmtpNetworkDeliveryNotSupported</GeneratePlatformNotSupportedAssemblyMessage>

to the project file.

@akoeplinger
Copy link
Member Author

@stephentoub I can imagine someone wanting to create mail messages in the browser context and with this change the unit tests that create one in the file system work.

@stephentoub
Copy link
Member

I can imagine someone wanting to create mail messages in the browser context and with this change the unit tests that create one in the file system work.

Can you share what such a scenario looks like? What does someone do with a mail message they create in the browser's file system?

@akoeplinger
Copy link
Member Author

Can you share what such a scenario looks like? What does someone do with a mail message they create in the browser's file system?

Pass it to another library, inspect it locally, send it to an API, .... I don't have a super important use case in mind but it did work in the mono/mono WASM implementation and it was quick to make it work in dotnet/runtime so why not? 😄

@stephentoub
Copy link
Member

stephentoub commented Jun 22, 2020

it was quick to make it work in dotnet/runtime so why not?

Simply because it adds complication/complexity/maintenance burden, and it seems a bit far-fetched. But if you really believe it adds value, ok.

It's not supported on WebAssembly so throw PlatformNotSupportedException.
@akoeplinger akoeplinger changed the title WebAssembly: Refactor System.Net.Mail to make network support optional WebAssembly: Add PNSE for System.Net.Mail Jul 3, 2020
@akoeplinger
Copy link
Member Author

@stephentoub I changed the PR to throw PNSE instead.

@akoeplinger akoeplinger merged commit 80fe5bb into dotnet:master Jul 3, 2020
@akoeplinger akoeplinger deleted the wasm-sys-net-mail branch July 3, 2020 14:51
Copy link
Member

@stephentoub stephentoub left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks.

@karelz karelz added this to the 5.0.0 milestone Aug 18, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants