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

rely less on Requires.jl #478

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

rafaqz
Copy link

@rafaqz rafaqz commented Jan 20, 2022

This PR reduces the use of Requires.jl.

  • it moves WebSockets code out of Requires, its a direct dependency anyway
  • Adds a dependency to Mux.jl. Its small and shares most deps, and really makes things faster having it precompiled.
  • Removes Blink.jl completely. Blink depends on WebIO and can define its own WebIO methods, which will also be precompiled. WebIO support is already implemented in Blink.jl

This needs a companion PR to Blink.jl, to do once this is approved. Overall this PR improves Blink.jl load time from 5s to 1s, on my laptop.

Closes #476

See JuliaGizmos/Blink.jl#288

This PR will also need a minor version bump to allow updating Blink.jl

Turns out the changes have already been made in Blink.jl, its the other two things slowing it down.

@twavv
Copy link
Member

twavv commented Jan 21, 2022

I'll take a look at this this weekend.

@twavv
Copy link
Member

twavv commented Jan 22, 2022

This looks decent to me. So no actual code changes are needed for Blink.jl? If that's the case, this can just be a patch release.

@rafaqz
Copy link
Author

rafaqz commented Jan 23, 2022

Maybe you forgot about this one

JuliaGizmos/Blink.jl#201

It seems to work for me anyway

@rafaqz
Copy link
Author

rafaqz commented Jan 23, 2022

I cant get the tests running locally, the npm call doesn't spawn. But if you can approve the CI here we can see if it runs

@codecov
Copy link

codecov bot commented Jan 23, 2022

Codecov Report

Merging #478 (4911003) into master (91156f8) will increase coverage by 1.04%.
The diff coverage is 80.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #478      +/-   ##
==========================================
+ Coverage   59.70%   60.75%   +1.04%     
==========================================
  Files          16       15       -1     
  Lines         757      739      -18     
==========================================
- Hits          452      449       -3     
+ Misses        305      290      -15     
Impacted Files Coverage Δ
src/node.jl 88.40% <ø> (ø)
src/WebIO.jl 72.00% <66.66%> (-8.00%) ⬇️
src/providers/generic_http.jl 73.77% <100.00%> (ø)
src/render.jl 29.54% <0.00%> (-2.28%) ⬇️
src/asset.jl 81.66% <0.00%> (+1.66%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 91156f8...4911003. Read the comment docs.

@rafaqz
Copy link
Author

rafaqz commented Jan 24, 2022

I dont really understand that documentation failure. Any ideas?

@rafaqz
Copy link
Author

rafaqz commented Jan 27, 2022

Ok I think I've fixed this, if you can try running it again. I have a type stability PR ready too that builds on this, so if we can get this merged I can get that started.

@rafaqz
Copy link
Author

rafaqz commented Feb 3, 2022

bump

@rafaqz
Copy link
Author

rafaqz commented Feb 4, 2022

@travigd can you switch the github actions permissions over to "Require approval for first-time contributors"?

It's hard to debug the docs problems currently

@rafaqz
Copy link
Author

rafaqz commented Feb 20, 2022

@travigd lets get this merged before I forget about it. There is a problem with building the docs on github actions.

If you change the github actions permissions over as mentioned above I can get it passing. Approving every CI run on every push to JuliaGizmos PRs doesn't seem to be a good use of your time to me!

Now all the other PRs are registered, this PR still reduces the time to both first Interact.jl slider and Blink..jl Window by ~1 second, depending on your machine.

It's not as much as before, because a lot of the time was Parsers.jl, and that's fixed now. But still a nice improvement.

@twavv
Copy link
Member

twavv commented Feb 22, 2022

Sorry for being incommunicado recently. My life has been hectic and I use Julia very little in my day-to-day work life nowadays.

The docs build is failing because the build step fails (or just isn't run?) in CI. There are a few possible solutions:

  • Build it in the CI for docs. This would require install nodejs so it can run all the JS toolchain stuff.
  • Change the error(...) in __init__ to be a log warning (e.g., @error).
  • Have the bundle_key ref be initialized lazily as a function:
    _bundle_key_ref = Ref{String}("")
    function _bundle_key()
      current = _bundle_key_ref[]
      if current == ""
        # try to initialize it here
      end
      return current
    end
    
    # later
    string(base, _bundle_key())

I'm partial to the third option.

(also FYI, I had to set it to only restrict actions for accounts new to github, not just new contributors, since you don't actually count as a contributor until something is merged)

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.

Rethink @requires dependencies
2 participants