Skip to content

Provide custom fallback instead of setTimeout #50

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

Closed
cornerman opened this issue Apr 16, 2022 · 13 comments
Closed

Provide custom fallback instead of setTimeout #50

cornerman opened this issue Apr 16, 2022 · 13 comments

Comments

@cornerman
Copy link

I am bit afraid about the fallback to setTimeout. This would cause serious performance issues. In most cases postMessage should be there in browsers and we have setImmediate in nodejs. But if not, i might decide to fallback to the unfair promise queue or throw an exception depending an the use-case.

Should we make this configurable or will it "never" happen? :)

@armanbilge
Copy link
Member

This project is based on the popular JS setImmediate polyfill, which falls back to setTimeout.

https://github.com/YuzuJS/setImmediate

So, it must be good enough?

@djspiewak
Copy link
Collaborator

I think the problem is that the unfair promise based executor will cause performance problems of a different and less diagnoseable sort. We've seen this a few times in various guises. At least setTimeout doesn't break the UI rendering, even if it is very sluggish.

@cornerman
Copy link
Author

cornerman commented Apr 16, 2022

Thanks for your answers. Agree that the promise based executor is not really desirable. Though depending on the case, it might be a better performing fallback then setTimeout. But of course not generally correct.

My question is probably more: how often will the fallback actually happen? And should we have a way to detect it in the application using this project - so they can emit a warning to their users?

@armanbilge
Copy link
Member

armanbilge commented Apr 16, 2022

Well, postMessage is available to 94% of users according to https://caniuse.com/mdn-api_window_postmessage

Notably, Promise itself is only available to 94% of users as well:
https://caniuse.com/promises

So ... :)

The setImmediate polyfill has nearly 14 million weekly downloads. Are there any open issues on that project that particularly concern you?

@cornerman
Copy link
Author

True, support seems to be quite widespread. And good point on comparing it to promise support.

I am just afraid and want to make sure, I do not end up with using setTimeout. That is why I am asking to at least have a way to detect this fallback.

@armanbilge
Copy link
Member

armanbilge commented Apr 16, 2022

I mean, what do you want to do in that situation? Since in those situations it seems likely that setTimeout is the only option anyway.

Note that even the default EC in Scala.js also falls back to setTimeout.
https://github.com/scala-js/scala-js/blob/e41949e96b91fb4d48e65e3cd9a15b57e518d673/library/src/main/scala/scala/scalajs/concurrent/JSExecutionContext.scala#L28

If you're looking for a mechanism to detect old, unsupported browsers for your application, it seems you should do exactly that. Which I think is out-of-scope for this project.

@cornerman
Copy link
Author

cornerman commented Apr 16, 2022

I would probably think about warning the user of degraded performance. Detecting outdated browsers could be an alternative approach to that indeed.

@armanbilge
Copy link
Member

armanbilge commented Apr 16, 2022

Right. I guess my point is, your concerns are almost definitely broader than what this particular project does or does not.

For example in outwatch/outwatch#605 you switched to using microtasks for snabbdom patching. What happens if neither queueMicrotask nor Promise are available in the browser? IIUC your application will completely break, which I would personally consider a serious form of "degraded performance" 😉

https://github.com/cornerman/colibri/blob/d4b42be624e70b982511c4d0d31fbd34b8509532/colibri/src/main/scala/colibri/helpers/NativeTypes.scala#L37

@cornerman
Copy link
Author

cornerman commented Apr 16, 2022

Right, outwatch does not handle it gracefully if promises are not defined. I thought of falling back to patching synchronously but i am not sure. I guess, this will not be supported for now. And colibri also needs the promise queue. So, you are right: for that use-case i would need a more sophisticated detection :)

At least, i got rid of the linked native types to using the MacroTaskExecutor and the scalajs promise executor.

But apart from outwatch and apart from the promise queue, I still think it would be useful to at least know when setTimeout is used as a fallback in this project.

@armanbilge
Copy link
Member

armanbilge commented Apr 16, 2022

Ok, that's fair. Adding to the API is a bigger question, but I suppose adding a console.warn(...) is reasonable. Even more so if we only do it in development mode. Do you think that would work for you?

@cornerman
Copy link
Author

I think that would be a good solution. I like it! It will make the issue visible :)

@armanbilge
Copy link
Member

armanbilge commented Apr 28, 2022

I think the problem is that the unfair promise based executor will cause performance problems of a different and less diagnoseable sort. We've seen this a few times in various guises. At least setTimeout doesn't break the UI rendering, even if it is very sluggish.

I think this really nails it on the head. The example in the README demonstrates that the worst-case for the Promises-based executor is that the UI will completely stop responding. It is easy to fall into this trap when writing idiomatic Scala, for example in some library that cross-builds for Scala.js without explicitly being designed for it. That's why the MacrotaskExecutor is the right general-purpose default.

Meanwhile, the worst-case for the MacrotaskExecutor is that it will be sluggish, for a fraction-of-a-percent of users.

If you are very concerned about the 0.2% of users for which a high-performance macrotask polyfill is not available (and not concerned about the 0.2% of users that don't have Promises support either) then you can explicitly run and test that code on the Promises-executor instead. Ideally, this should be scoped to small regions of your application that you are confident will not create serious fairness issues.

@cornerman
Copy link
Author

I think, you are right. I was also reflecting on this topic in the past days and now agree that it is actually fine as it is. Thank you so much for all the answers and interest in this topic 👍

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 a pull request may close this issue.

3 participants