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

Replace webpack with require.js #1554

Closed
minrk opened this issue Jun 17, 2016 · 17 comments · Fixed by #2011
Closed

Replace webpack with require.js #1554

minrk opened this issue Jun 17, 2016 · 17 comments · Fixed by #2011
Projects
Milestone

Comments

@minrk
Copy link
Member

minrk commented Jun 17, 2016

Proposal from #1547: go back to using require.js for this notebook, and leave webpack for newer projects, such as jupyterlab.

The switch to webpack in the legacy notebook seems to make lots of backward-compatibility maintenance impossible, and has been the source of numerous issues (#1547, #1439, #1431, etc.). Following some discussion in #1547, I think it's not feasible to maintain our goals of backward-compatibility in the existing notebook javascript while adopting webpack. The key point being that it seems to be impossible to include a module in the webpack, and then load it later with require, which is a necessity for numerous extensions and official APIs.

If that's not true, and we are just failing to configure things correctly, there might be a nicer, simpler fix. But this gist is, for the notebook js the following two things must return the same object:

  • require('notebook/js/cell') in our packed-up sources
  • require('notebook/js/cell') at runtime, in outputs, custom.js, or nbextensions

(and so on, for codemirror and everything else we load via require). The behavior in master is to load a different copy of the module, which is causing all kinds of things to break.

@minrk minrk added this to the 5.0 milestone Jun 17, 2016
@blink1073
Copy link
Member

So far we have limped along by pulling things out of the WebPack bundle that might be required using RequireJS (jQuery, CodeMirror). For things that should be in the bundle (like notebook/js/cell), we can't pull those out to be external, but we could export them from the bundle and manually point the RequireJS path to the bundle, but I do not see that scaling for all intended uses and would be a whack-a-mole solution.

@minrk
Copy link
Member Author

minrk commented Jun 17, 2016

Yeah, and that's no good because everything should be loadable by require—certainly notebook, cell, outputarea, etc. We are only bundling them for load-time optimization. When we did the bundling with require, it had no effect on the behavior, only an optimization. Pulling things out to script tags undoes all that.

@blink1073
Copy link
Member

blink1073 commented Jun 17, 2016

Technically we could use a DllPlugin to create the shared bundle, use the DllReferencePlugin to consume the bundle from Webpack, and find/write a way to generate a RequireJS config using the manifest created by the DllPlugin.

@minrk
Copy link
Member Author

minrk commented Jun 17, 2016

We could try that. I'm not sure if there's a benefit to using webpack in this repo given our backward-compatbility goals, though. I think the main reason was that it's what we want to use in new projects, so we should be consistent, but with lab in its own repo I think that makes less sense now.

Are there benefits to webpack over require.js in this repo?

@blink1073
Copy link
Member

blink1073 commented Jun 17, 2016

I'm looking at this from the opposite side as well, how do we load external things in JupyterLab, or do we want to force everything to be WebPacked? That would mean, for instance, that installing/uninstalling an extension would require a rebuild of WebPack. We could still support dynamic enable/disable by using the html template to define the bootstrapping code for the application, and use the desired modules from the bundle.

@minrk
Copy link
Member Author

minrk commented Jun 17, 2016

installing/uninstalling an extension would require a rebuild of WebPack

I think that's not going to work. We cannot require running webpack to be a runtime dependency for using an extension.

I expect that we should be able to deal with this in JupyterLab by making the extension points more clearly defined exported functions / events / observables / etc., Rather than allowing full access to everything in extensions. If we do that, extensions shouldn't need to be able to extend prototypes on Cell, etc. which is the source of the problems we have in the notebook right now (and then the fact that they cannot extend these prototypes becomes a good thing).

@ellisonbg
Copy link
Contributor

I am fine backing out of using webpack in the classic notebook...

On Fri, Jun 17, 2016 at 3:16 AM, Min RK notifications@github.com wrote:

installing/uninstalling an extension would require a rebuild of WebPack

I think that's not going to work. We cannot require running webpack to be
a runtime dependency for using an extension.

I expect that we should be able to deal with this in JupyterLab by making
the extension points more clearly defined exported functions / events /
observables / etc., Rather than allowing full access to everything in
extensions. If we do that, extensions shouldn't need to be able to extend
prototypes on Cell, etc. which is the source of the problems we have in the
notebook right now (and then the fact that they cannot extend these
prototypes becomes a good thing).


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#1554 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AABr0H0seYFNNHDgbHsfvXoO7PDjNYsYks5qMp4TgaJpZM4I4JjG
.

Brian E. Granger
Associate Professor of Physics and Data Science
Cal Poly State University, San Luis Obispo
@ellisonbg on Twitter and GitHub
bgranger@calpoly.edu and ellisonbg@gmail.com

@blink1073
Copy link
Member

blink1073 commented Aug 15, 2016

I had thought we could rectify the use of Webpack and RequireJS, but existing extensions rely on the ability to require() individual modules of the Notebook and modify them. The changes being made to Phosphor here enable us to do so from code that will be Webpacked itself and use the appropriate Webpack externals config.

I recommend taking the stance that the Notebook is and always will be AMD-only and use RequireJS.

@blink1073
Copy link
Member

@gnestor, thoughts on this?

@gnestor
Copy link
Contributor

gnestor commented Aug 16, 2016

Let me start out by saying that I don't have any experience using AMD and RequireJS and I have a lot of experience using CommonJS and Webpack. Webpack is great because it allows you to use a lot of the good things about Node.js (CommonJS, npm) within the browser by bundling all of your modules into a file that can be used within a <script> tag. With Webpack, it's easy to use external modules (such as browser libraries like jQuery and Bootstrap) from within the bundle without actually bundling them using the externals config. This would allow the core notebook (bundled by Webpack) to access extensions (not bundled). However, I don't know of any good/safe way to allow extensions to access the notebook core, but like @blink1073 mentioned, there are a couple Webpack plugins that would enable this:

  • DllPlugin and DllReferencePlugin: Webpack plugin that allows one bundle to provide modules (e.g. vendor bundle) to another consumer bundle (e.g. app bundle). I couldn't find an example of consuming a DLL bundle outside of Webpack (only examples of a provider bundle using the DllPlugin and a consumer using the DllReferencePlugin).
  • script-loader: Webpack loader that will evaluate a module in the global context (like a <script> tag).
  • expose-loader: Webpack loader that will export a module to the global context.

It appears that all of these options would enable extensions to access the notebook core by exposing the notebook core to the global context, which is not a good solution.

I expect that we should be able to deal with this in JupyterLab by making the extension points more clearly defined exported functions / events / observables / etc., Rather than allowing full access to everything in extensions. If we do that, extensions shouldn't need to be able to extend prototypes on Cell, etc. which is the source of the problems we have in the notebook right now (and then the fact that they cannot extend these prototypes becomes a good thing).

I have experience developing Atom packages and if you look at Atom's API, it also exposes an atom global that allows packages to subscribe to events, get state, and safely set state. Instead of "extending the prototype of a [TextEditor]," a package can render custom UI within a TextEditor by calling a method on that editor.

I'm not familiar with the current notebook- or jupyterlab-extension API, so I would need to see some docs/examples to offer any suggestions regarding to Webpack or not to Webpack.

@rgbkrk I know that nteract is using Electron (the same native shell that Atom uses) and therefore doesn't require Webpack, but do you have any insights nonetheless?

@rgbkrk
Copy link
Member

rgbkrk commented Aug 16, 2016

Seems like we can never divest from requirejs based on previous usage of and reliance on AMD. I'll go ahead and stand by @blink1073 in saying "that the Notebook is and always will be AMD-only and use RequireJS.", while hanging my head 😢. Thanks for giving it a go.

As for nteract and Electron, we face the same problem as in jupyter/roadmap#9 and #116, regardless of the module loader. I don't have any solutions though.

@rgbkrk
Copy link
Member

rgbkrk commented Aug 16, 2016

/cc @jdfreder

@blink1073
Copy link
Member

Proposal: use the DllPlugins for the notebook itself and use the resulting manifest to generate define blocks for each of the modules to inject into page.html. The define blocks would look like define('base/js/namespace', function() { return main_bd2114230b51144d70d3(42); });, where main_bd2114230b51144d70d3 was the name in the manifest and 42 was the module number for base/js/namespace.

@gnestor
Copy link
Contributor

gnestor commented Sep 22, 2016

@blink1073 This sounds like it's worth a try. I'm happy to help although I think you have a better understanding of how to go about implementing it. Do you want to give it a shot? Also, what is the priority level of this (1 being this should be worked on whenever and 5 being this should be worked on immediately)? @minrk Thoughts?

@blink1073
Copy link
Member

I'll take a stab at it next week.

@minrk
Copy link
Member Author

minrk commented Sep 23, 2016

Sounds like it's worth a shot. A while back, I took a stab at reverting to the old requirejs setup entirely for the legacy notebook, but too many webkit-based things have crept in that made it pretty difficult, and I had to move on to other things.

I don't care about the build tools too much, as long as we can fix the runtime-require behavior that users are already relying upon.

@gnestor
Copy link
Contributor

gnestor commented Dec 22, 2016

In the interest of releasing 5.0 soon, I think it's time to act on this. I will certainly help or take the lead on this if necessary. Here is the webpack PR with 36 commits! I don't know if there is a semi-automatic way to revert a PR/merge, but if there is that could be a good place to start. Thoughts?

@minrk minrk mentioned this issue Jan 6, 2017
1 task
@gnestor gnestor moved this from Open issues to Closed issues in 5.0 Feb 4, 2017
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
5.0
Closed issues
Development

Successfully merging a pull request may close this issue.

5 participants