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

Make body!() and content!() synchronous. #136

Merged
merged 2 commits into from
Aug 15, 2018

Conversation

NHDaly
Copy link
Collaborator

@NHDaly NHDaly commented Jul 27, 2018

Adds an option, async to body!() and content!(), which when true will
give the old behavior (return immediately), but when false, will block until
the Window is finished evaling the javascript statement to assign the new content.

Note that this blocks until evalscripts has been called for all new scripts,
but that if your scripts themselves do anything asynchronous, it won't wait for
them to finish.


Right now, I have async=true by default so that this doesn't change the
current behavior, but I really think we should set this false by default! :) If
you want, we can merge it in like this, wait for it to soak in, make sure
everyone likes the change, etc, and then flip the switch?

That said, it shouldn't break anyone's code to just flip the default, it will
just likely make it a bit slower.

Part of fixing #108.

@NHDaly
Copy link
Collaborator Author

NHDaly commented Jul 27, 2018

A couple questions:

  1. The only remaining content api that doesn't have an async option is loadcss!. Can we simply change that to use @js instead of @js_?
  2. Before this PR, content! called @js_, which returns the Page it's called with. I guess this could be useful for chaining? (e.g. content!(content!(w, "#a", "x"), "#b", "y")), but idk why you'd do that..
    So for this PR, I kept it that way, even in the async version. Do you think that makes sense, or is it better to return something like true showing it succeeded or something?

@NHDaly
Copy link
Collaborator Author

NHDaly commented Jul 27, 2018

After this, I think the only remaining major API that doesn't have a synchronous option is opening a Window. It's annoying that that isn't synchronous as well, so i'd like to tackle that too, but it'll have to be a different PR. :)

NHDaly added 2 commits August 4, 2018 10:04
Removed 5 `sleep(1)` statements, and reduced test time by 4.5 seconds:

```
$ time julia test/content/api.jl  # Before
julia test/content/api.jl  15.07s user 0.33s system 28% cpu 54.958 total
$ time julia test/content/api.jl  # After
julia test/content/api.jl  15.60s user 0.35s system 31% cpu 51.197 total
```
@NHDaly
Copy link
Collaborator Author

NHDaly commented Aug 14, 2018

Friendly ping. This passes all tests. Is there any objection to me just merging this in? :]

@pfitzseb
Copy link
Member

LGTM.

@NHDaly
Copy link
Collaborator Author

NHDaly commented Aug 15, 2018

Cool, thanks a bunch! :)

@NHDaly NHDaly merged commit 3eff499 into JuliaGizmos:master Aug 15, 2018
@NHDaly NHDaly deleted the synchronous branch August 15, 2018 12:45
@pfitzseb
Copy link
Member

Thanks for the PR! :)

@NHDaly
Copy link
Collaborator Author

NHDaly commented Aug 15, 2018

:) No problem!

Also, i've just realized it's worth noting that I left async=true as the default, so this doesn't change anything by default. Users have to manually pass async=false to get the synchronous behavior.

That said, I think probably false should be the default, since A) it's almost always probably what you want, and B) that matches most normal julia functions, which are synchronous by default.

I think probably the best way to proceed is after also checking-in #137, we have a single PR that flips the defaults. I think probably that should happen with a version bump, though, since it changes behavior. Does that sound right to you? :)

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.

2 participants