Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Avoid using unsupported APIs #5057

Merged
merged 7 commits into from
Feb 19, 2025

Conversation

Youssef1313
Copy link
Member

No description provided.

@Youssef1313 Youssef1313 marked this pull request as ready for review February 19, 2025 08:15
@Evangelink Evangelink enabled auto-merge (squash) February 19, 2025 08:16
Comment on lines +291 to +295
private void ConsoleWarn(string? message) => _console.WriteLine(message);

private void ConsoleError(string? message) => _console.WriteLine(message);

private void ConsoleLog(string? message) => _console.WriteLine(message);
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion that doesn't need to be done here. I know nothing about web so maybe that's totally stupid. Is it common to have some kind of prefix for warning/errors in the console? If so we may want to prefix the message

Copy link
Member Author

@Youssef1313 Youssef1313 Feb 19, 2025

Choose a reason for hiding this comment

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

@Evangelink Usually we would want to use the right API (e.g, console.error and console.warning), which is already done when JSImport is available (net7+).

Ideally, we will want to still always call console.error and console.warning for all TFMs.

@captainsafia Sorry for the ping here, but I can't seem to find the "old" way of doing JS interop as probably all docs are updated to the source-generated attributes. Do you know how to call console.error and console.warning when the source generator isn't available? (Note: we are a netstandard2.0 and net6+ library)

Copy link
Member

Choose a reason for hiding this comment

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

I don't know off the top of my head but @MackinnonBuck might have ideas here.

@Evangelink
Copy link
Member

As discussed offline with @Youssef1313, let's merge as-is and follow up

@Evangelink Evangelink disabled auto-merge February 19, 2025 15:09
@Evangelink Evangelink merged commit 1d72cd0 into microsoft:main Feb 19, 2025
8 checks passed
@Youssef1313 Youssef1313 deleted the unsupported-platform branch February 19, 2025 15:16
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants