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

Fix JS imports on JuliaBox #99

Merged
merged 10 commits into from
May 6, 2018
Merged

Conversation

rdeits
Copy link
Collaborator

@rdeits rdeits commented May 3, 2018

Supersedes #97, fixes #98, fixes #96, fixes #101

Edit: now rebased on #103

@codecov-io
Copy link

codecov-io commented May 3, 2018

Codecov Report

Merging #99 into master will increase coverage by 0.15%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #99      +/-   ##
==========================================
+ Coverage    60.8%   60.95%   +0.15%     
==========================================
  Files          14       14              
  Lines         398      397       -1     
==========================================
  Hits          242      242              
+ Misses        156      155       -1
Impacted Files Coverage Δ
src/iframe.jl 0% <ø> (ø) ⬆️
src/devsetup.jl 0% <0%> (ø) ⬆️

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 204f580...440e8dd. Read the comment docs.

@rdeits rdeits requested a review from shashi May 3, 2018 15:24
@shashi
Copy link
Member

shashi commented May 3, 2018

I'm fine with the diff for now. We should look into fixing the webpack version that runs on bundlejs().

Awesome improvements btw! Thank you.

@djsegal does this still keep binder working?

@rdeits rdeits mentioned this pull request May 3, 2018
4 tasks
@rdeits
Copy link
Collaborator Author

rdeits commented May 3, 2018

Hm, there's something wrong here. When I try to test MeshCat.jl over WebIO locally, I get Error: cannot find module "vm" in the javascript console. I suspect something has gone wrong in the bundling process.

@djsegal
Copy link
Contributor

djsegal commented May 3, 2018

I'll try to check this out tonight! Are there any reader's digest tips about what to look out for?

@rdeits
Copy link
Collaborator Author

rdeits commented May 3, 2018

@djsegal you might want to wait to test this out. There's something wrong with the bundling process, at least on my machine (see #101 ). It's happening for me on master and also on this PR, so it will make testing harder.

@rdeits
Copy link
Collaborator Author

rdeits commented May 4, 2018

Sigh. Weirdly, the authorization: true flag breaks WebIO scope imports of external JS files in JupyterLab. For example, after rebuilding the labextension with jupyter lab build the p5.js demo from the Readme no longer works, and instead the console shows:

Error: NetworkError when attempting to fetch resource.
  Instantiating http://cdnjs.cloudflare.com/ajax/libs/p5.js/0.5.11/p5.js
  Loading http://cdnjs.cloudflare.com/ajax/libs/p5.js/0.5.11/p5.js

@rdeits
Copy link
Collaborator Author

rdeits commented May 4, 2018

Safest thing to do seems to be to only set {authorization: true} for local pkg imports.

@shashi
Copy link
Member

shashi commented May 4, 2018

That sounds OK.

@shashi
Copy link
Member

shashi commented May 4, 2018

from this discussion systemjs/systemjs#1150 it seems like {authorization: true} sends the cookies in the XHR request. It probably sends the same to external CDN servers as well, which they are at full liberty to refuse to respond to.

I'm alarmed now by the extra 7k lines in this diff!! Let's try and fix that before merging.... I can try building it on my local setup when you're done editing.

@shashi
Copy link
Member

shashi commented May 4, 2018

I see most of it is package-lock.json... Did you figure out if we can go back to webpack 0.21?

@rdeits
Copy link
Collaborator Author

rdeits commented May 4, 2018

Ok, sounds good. The extra 7k lines are all because I checked in package-lock.json (which is recommended). Actually, the changes to the js bundles are probably bigger but have fewer line changes because webpack squishes everything into a single line.

But if you want to minimize the impact of this PR, I can remove package-lock.json from this PR and we can discuss committing it later.

@rdeits
Copy link
Collaborator Author

rdeits commented May 4, 2018

I get the Critical dependency: require function is used in a way in which dependencies cannot be statically extracted warning for systemjs 0.21.3, 0.21.2, and 0.21.1. It looks like 0.21.0 is OK, though. I suspect that was the version available when the bundle was last successfully built (it was the latest version from mid-January through the end of March 2018). I'll update the package.json to use that version.

@rdeits
Copy link
Collaborator Author

rdeits commented May 4, 2018

systemjs/systemjs#1817

@shashi
Copy link
Member

shashi commented May 5, 2018

I'm ok to merge this and wait for resolution of the SystemJS issue. Then we can try and upgrade.

Thanks a million for looking into these issues. This is great!

@rdeits
Copy link
Collaborator Author

rdeits commented May 5, 2018

Ok, sounds good. This includes all the changes from #103 as well, so this is the only PR that needs to be merged. I'd suggest doing a squash merge for this, since otherwise we'll accumulate a bunch of noisy changes in bundle.js from my various attempts at regenerating it.

@djsegal this should now be ready to test on binder, if you have a chance. I'd suggest just running through the examples from the readme and making sure they all run.

@shashi shashi merged commit bfe8220 into JuliaGizmos:master May 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants