Skip to content

BlazorWasm support - #256

Merged
slozier merged 3 commits into
IronLanguages:masterfrom
TwoUnderscorez:wasm-support
Apr 5, 2022
Merged

slozier merged 3 commits into
IronLanguages:masterfrom
TwoUnderscorez:wasm-support

Conversation

@TwoUnderscorez

Copy link
Copy Markdown
Contributor

When trying to use IronPython3 from a Blazor web-assembly application, Microsoft.Scripting.Runtime.SharedIO throws an exception when trying to setup stdin, which is understandable, because there is none.

All I did is catch the exception and set the stream to Stream.Null.

@dnfadmin

dnfadmin commented Feb 22, 2022

Copy link
Copy Markdown

CLA assistant check
All CLA requirements met.

@TwoUnderscorez
TwoUnderscorez marked this pull request as ready for review February 22, 2022 15:32

@slozier slozier left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the PR. My main concern with this approach is that if someone changes the order of the assignments (I think) it'll throw a PNSE instead of a TypeInitializationException. Probably better to fix ConsoleInputStream to not throw on initialization and then catch the PNSE.

I guess my other problem is I don't have a Blazor test rig to validate this. Any pointers on setting one up?

Related issue: IronLanguages/ironpython2#769
Workaround: IronLanguages/ironpython2#769 (comment)

@TwoUnderscorez

Copy link
Copy Markdown
Contributor Author

@slozier Is that better?

@slozier slozier left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks, that works.

Going from a field to a property is a breaking change but I assume no one outside the assembly is using this... Also I think we lose thread safety on the singleton creation. Will need to look into it a bit more, but I guess I can fix it after the merge if required.

@TwoUnderscorez

Copy link
Copy Markdown
Contributor Author

@slozier Sorry, I don't quite understand how this is a breaking change. If you would point me in the right direction I might be able to make it a non-breaking change.

@slozier

slozier commented Feb 26, 2022

Copy link
Copy Markdown
Contributor

@TwoUnderscorez I meant having ConsoleInputStream.Instance go from a field to a property is technically a binary breaking change (for example if assembly A uses a field from assembly B and you replace B with B' which contains a property instead, then A is broken). I don't think I'm really worried about breaking anyone with this particular scenario since the only consumer is in the same assembly - but it is a public type so it's not impossible...

An alternative approach would be to leave ConsoleInputStream.Instance as a field and change the constructor to not throw:

private ConsoleInputStream() {
    try {
        _input = Console.OpenStandardInput();
    } catch (PlatformNotSupportedException) {
        _input = Stream.Null;
    }
}

@TwoUnderscorez

Copy link
Copy Markdown
Contributor Author

@slozier I could do that, but I would have to leave the catch block in ShareIO.InitializeInput because

_inputEncoding = Console.InputEncoding;
_inputReader = Console.In;

also throw PlatformNotSupportedException (maybe only one of them), which I personally think is kind of ugly. Would you like me to do what you've suggested or merge as is currently?

@slozier

slozier commented Mar 10, 2022

Copy link
Copy Markdown
Contributor

@TwoUnderscorez I think the PNSE try/catch you have in InitializeInput is fine, but I'd change the ConsoleInputStream to what I suggested instead.

@slozier
slozier merged commit 910e6a0 into IronLanguages:master Apr 5, 2022
@slozier

slozier commented Apr 5, 2022

Copy link
Copy Markdown
Contributor

Thanks for the PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants