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

Warn if falling back to setTimeout in dev mode #51

Closed
wants to merge 1 commit into from

Conversation

armanbilge
Copy link
Member

Closes #50. /cc @cornerman

Thoughts?

Comment on lines +156 to +161
if (LinkingInfo.developmentMode) {
if (js.typeOf(js.Dynamic.global.console) != Undefined && js.typeOf(js.Dynamic.global.console.warn) != Undefined)
js.Dynamic.global.console.warn(
"Unable to polyfill setImmediate() in this environment so falling back to setTimeout(); this may affect performance. " +
"See https://github.com/scala-js/scala-js-macrotask-executor for more information.")
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Trying to balance being helpful/informative without being too alarmist, since this is still a correct implementation.

@sjrd
Copy link
Member

sjrd commented Apr 16, 2022

TBH, I think this is entirely the wrong thing to do. If I depend on some core library like this, the last thing I want it to do is for it to print stuff to the console without my knowledge, and without me being able to turn it off or choose to display the information in a different way.

Moreover, I don't think anyone will actually see this, and hence the point will likely be lost. The developer won't see it because they're likely using only environments where it doesn't show up. Users won't see it because they will get the production version, and anyway they don't look at the console.


If this is somehow necessary, I would much rather have an API point that returns a description of what method is actually being used. An enum-like thing could work, though it's important to make it evolvable in binary compatible ways.

@cornerman
Copy link

I would love to get a feedback on which method is used by the executor. Logging to the console in case of setTimeout seems better than nothing. If it is a method returning what is used, that is even better :)

@djspiewak
Copy link
Collaborator

Maybe a better approach would be to have a global flag which disables the fallback altogether? So if you want to either have high performance or nothing, you could get that semantic.

@armanbilge
Copy link
Member Author

armanbilge commented Apr 16, 2022

Thanks for the feedback. Overall I agree with #51 (comment) which is why I tucked this under dev-mode only.

OTOH, personally I'm not convinced this is necessary enough to expose in the API itself given the constraints of maintaining backwards-compatibility. See also discussion in #37 (comment) about the fact that how this ExecutionContext schedules macrotasks is an implementation detail.

Btw, all of this still feels pretty hypothetical. To at least make this concrete, is there an environment where such a thing would be useful, as in we legitimately cannot implement this polyfill but we can use Promise, and a reasonable Scala.js program can run without getting tripped up by other missing APIs?

@cornerman
Copy link

My motivation to open the issue was mainly based on fear of falling back to a badly performing solution. I have no feedback on what method is used. I can check which browsers are supported but there was some uncertainty for me. This might lead to users complaining about the performance of the application. I feel it would be better to tell the user that their platform is not supported then.

I cannot really say how widespread this problem actually is. Also whether other APIs that we are using would be problematic for these users as well, as @armanbilge mentioned already. Is there an understanding which browsers/runtimes our scalajs libraries actually support when using macrotaskexecutor or securerandom? Maybe the affected userbase is actually so small that it is not worth handling it at all.

There was also this issue, where people were wondering which method will be used in browser extension: #38 (comment). This is why i thought a console warning might be a good step.

It could also be that we need to define what this library does: is it just providing a correct polyfill or a high-performant and correct one. As @djspiewak said, a flag to say what you want or even two different executors - with and without fallback - might be a solution.

@armanbilge
Copy link
Member Author

armanbilge commented Apr 18, 2022

Thanks, those are great questions!

Is there an understanding which browsers/runtimes our scalajs libraries actually support when using macrotaskexecutor or securerandom?

Yes :)

First of all, MacrotaskExecutor supports any environment that provides setTimeout, since this is the fallback.
https://caniuse.com/mdn-api_settimeout

But, probably your question is specifically which environments will have "decent" performance:

This is all documented here:
https://github.com/scala-js/scala-js-macrotask-executor#performance-notes

Out of scope for this discussion, but similarly, SecureRandom supports:

Which is documented here:
https://github.com/scala-js/scala-js-java-securerandom#usage

There was also this issue, where people were wondering which method will be used in browser extension: #38 (comment)

Actually, I would summarize that issue as not about which method will be used, but rather about whether the MacrotaskExecutor would behave correctly in that environment and whether it would be performant.

The problem is that same-named methods can behave differently depending on the environment, so simply knowing which method is selected is not sufficient. For example:

Unfortunately, postMessage has completely different semantics inside web workers, and so cannot be used there.

https://github.com/YuzuJS/setImmediate#messagechannel

The ideal way to verify that the MacrotaskExecutor behaves correctly (and performantly) in that environment would be to run the test suite which checks both of those things.

It could also be that we need to define what this library does: is it just providing a correct polyfill or a high-performant and correct one.

The first goal of this library is to provide a correct implementation of an ExecutionContext that uses macrotasks. The second goal is to provide a high-performant one, but the first goal takes priority.

So far, we've been able to achieve both goals for all the environments tested in CI, and personally I'd be 👍 to any PRs that add more. Perhaps, that is the best way to address your uncertainties? :)

I hope that helps!

@armanbilge
Copy link
Member Author

Btw, if we really look at those caniuse numbers we have:

API Global %
postMessage 94.1%
Promise 94.3%
setTimeout 94.5%

So odds are, we are really talking about a fraction of a percent here.

@armanbilge armanbilge closed this Apr 28, 2022
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.

Provide custom fallback instead of setTimeout
4 participants