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

Remove requirejs dependency #1009

Merged
merged 1 commit into from
Oct 13, 2017
Merged

Conversation

lgeiger
Copy link
Member

@lgeiger lgeiger commented Sep 25, 2017

While checking out #908 I realized we don't need requirejs at all. I tested it and it works fine without.

@nikitakit Was there a specific reason for requirejs to be globally available? If not I guess this fixes #793

@rgbkrk
Copy link
Member

rgbkrk commented Sep 25, 2017

I think this is so libraries that try to use requirejs end up using this instead of producing a JS error (since the classic notebook uses it).

@nikitakit
Copy link
Contributor

The entire shim here is based on the documentation for @jupyterlab/services: ideally there would be a way to not install/override any globals, but I did what the docs said was needed just to be safe.

In particular if you look here you'll see that requirejs is needed for Comm targets. So not currently something that hydrogen uses, though I would like to have comm/widgets support in hydrogen eventually.

I guess hydrogen could try removing it, though it would be our responsibility if anything breaks as a result. As far as I can tell the @jupyterlab/services maintainers do most of their testing with browser-based code, so the moment we do anything different from the nodejs examples they provide we become responsible for testing all the edge cases that could arise from using the library inside node.

@nikitakit
Copy link
Contributor

Just saw #908 (comment): based on this I think it may be possible to do away with the globals shim entirely.

@lgeiger
Copy link
Member Author

lgeiger commented Sep 26, 2017

@nikitakit Thanks for the explanation!

Since requirejs is over a MB big and potentially caused #793 I think we could give it a shot and remove it. It's aIso missing in the minimal node example from jupyterlab. If we're hitting strange bugs, we can always roll back. What do you think?

Just saw #908 (comment): based on this I think it may be possible to do away with the globals shim entirely.

I have a rough version of this mostly working, though I'm hitting some bugs on jupyters side. I'll try to submit a PR soon.

@nikitakit
Copy link
Contributor

@lgeiger I see, since there are actual problems then it makes sense to try removing it. Unfortunately I'm not sure I'll be able to help with testing this.

@rgbkrk
Copy link
Member

rgbkrk commented Oct 13, 2017

Should we go ahead and bring this in?

@lgeiger
Copy link
Member Author

lgeiger commented Oct 13, 2017

Should we go ahead and bring this in?

👍 I think we should try it and see how it goes.

@rgbkrk rgbkrk merged commit 21cacf5 into nteract:master Oct 13, 2017
@lgeiger lgeiger deleted the remove-requirejs branch October 13, 2017 06:27
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.

Uncaught Error: Tried loading "pdfjs-dist/build/pdf.worker" at /Applications/Atom.app/Contents/Re...
3 participants