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

Use NPM, node ES5, and browserify. #268

Merged
merged 54 commits into from
Aug 26, 2015
Merged

Use NPM, node ES5, and browserify. #268

merged 54 commits into from
Aug 26, 2015

Conversation

jdfreder
Copy link
Contributor

Gets rid of require.js Leaves require.js, only for backwards compatibility.
Moves a lot of install logic out of setup base
Allows us to use NPM packages directly in our JavaScript! ⚡
No longer minify JS

Pinging interested people @minrk @Carreau @takluyver @rgbkrk .

TODO:

  • Get working
  • Get tests working
  • Add pre/post install commands

@jdfreder
Copy link
Contributor Author

I put all of the large drone work in the first commit, later commits are where all the good review needed stuff is at...

@minrk
Copy link
Member

minrk commented Aug 11, 2015

This still builds concatenated js with browserify?

@jdfreder
Copy link
Contributor Author

Yeah it's concatenated. One problem with this approach is that it's currently backwards incompatible with require.js based code (i.e. ipywidgets). With a little more thought, and maybe leaving require.js as a dependency, I should be able to keep backwards compatibility.

@rgbkrk
Copy link
Member

rgbkrk commented Aug 11, 2015

This is crazy and I'm excited to try it out.

missing = [ m[len(prefix):] for m in missing ]
raise ValueError("The following required files are missing: %s" % missing)

log.warn("rebuilding js and css failed.)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing a quote at the end here (though there are other issues too).

@Carreau
Copy link
Member

Carreau commented Aug 11, 2015

How long is the build step ? Is there a way to not have a build step and bundle and separate file to iterate over the notebook quickly ? The 30 s build step is annoying. Potentially a dev mode where files are actually es6 ?

@jdfreder
Copy link
Contributor Author

@Carreau 9.080s, I plan on implementing a watch command too.

@rgbkrk
Copy link
Member

rgbkrk commented Aug 12, 2015

It will get better as we break things apart better. This is purely migrating what we have right now. We also don't have to do the full bundle if we don't want to (can split apart and only rebuild small steps). For now Jon is tackling the conversion so it works then we must get into a nice live reload quick differential rebuild stage.

@Carreau
Copy link
Member

Carreau commented Aug 12, 2015

That seem like a plan !

Also side not, you might want not to remove indentation, otherwise rebase
will be painful.
We can "just" de-indent in 1 PR which is obvious to review with ?w=0.

On Tue, Aug 11, 2015 at 6:17 PM, Kyle Kelley notifications@github.com
wrote:

It will get better as we break things apart better. This is purely
migrating what we have right now. We also don't have to do the full bundle
if we don't want to (can split apart and only rebuild small steps). For now
Jon is tackling the conversion so it works then we must get into a nice
live reload quick differential rebuild stage.


Reply to this email directly or view it on GitHub
#268 (comment).

@rgbkrk
Copy link
Member

rgbkrk commented Aug 12, 2015

Yay, build is working successfully. I'm getting roughly ~8s per full build time npm run build.

@rgbkrk
Copy link
Member

rgbkrk commented Aug 12, 2015

@minrk
Copy link
Member

minrk commented Aug 12, 2015

Awesome, thanks for investigating better ways to work with Javascript.

@jdfreder
Copy link
Contributor Author

Also side not, you might want not to remove indentation, otherwise rebase will be painful.

Thanks for the suggestion, I rebase -i editted the commit with the de-indent, re-indenting everything. This should make the diff much more readable! We can open up a dedicated de-indent PR later.

"jquery": "components/jquery#~2.0",
"jquery-ui": "components/jqueryui#~1.10",

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JSON doesn't support comments, so I'm leaving one here in GitHub. The following packages can be removed from bower and the setupbase check now, but I left them here for backwards compatibility... In case any users are loading them in via requirejs.

@jdfreder
Copy link
Contributor Author

Even though there are still some JS errors on the notebook page and the tests aren't passing yet, the apps are working now.

@jdfreder
Copy link
Contributor Author

To maintain backwards compatibility with JS extensions that use requirejs, we could use a modified version of this: https://github.com/guybedford/cjs

Alternatively, we could use r.js to go from commonjs->amd quickly - much quicker than the bundling operation we did before.

@jdfreder
Copy link
Contributor Author

@minrk @Carreau @rgbkrk should I continue to pursue this? I'm trying to gauge interest. I've figured out a way to make it backwards compatible with existing notebook extensions using require.js (i.e. ipywidgets), I'd need a custom require.js extension.

I'm just wondering, if got this to work, if it would be something that we'd be interested in merging soon (< month). This would allow us to proceed with the idea of a gradual refactor, instead of a complete rewrite. We could start by trying the output area Kyle and I have pushed.

Otherwise we can mothball this, and revisit later if it's just too soon...

@ellisonbg
Copy link
Contributor

Very cool! Is everyone agreed that we want to go this way rather than require.js? What about code that needs to be loaded asynchronously and not on first page load? If this can help us start to refactor in place and move more quickly, I think it would be a great thing.

@jdfreder
Copy link
Contributor Author

@ellisonbg , yup, that works now. I had to do that for contents. If you want code to load dynamically, you call requirejs instead of require. require is node require, and gets browserified, where requirejs and define pass through and work like normal.

@ellisonbg
Copy link
Contributor

Great! What type of review do you think this needs? It is big and risks bit
rotting quickly so it would be great to get this reviewed quickly (not
rushed though).

On Thu, Aug 13, 2015 at 2:33 PM, Jonathan Frederic <notifications@github.com

wrote:

@ellisonbg https://github.com/ellisonbg , yup, that works now. I had to
do that for contents. If you want code to load dynamically, you call
requirejs instead of require. require is node require, and gets
browserified, where requirejs and define pass through and work like
normal.


Reply to this email directly or view it on GitHub
#268 (comment).

Brian E. Granger
Cal Poly State University, San Luis Obispo
@ellisonbg on Twitter and GitHub
bgranger@calpoly.edu and ellisonbg@gmail.com

@jdfreder
Copy link
Contributor Author

Well the biggest thing I need is agreement on transpiling node/es6/ts -> web es5, here I'm doing node es5 -> browser es5 using browserify. With that, code review on a few files, like package.json, notebook/js/main.js, and notebook/js/notebook.js - I think they are a good sample of what's going on. If I get the green light with this, I'll add a require.js bit for backwards compatibility, because as-is require-ing single modules leads to errors, because this PR changes the modules from AMD to commonjs. Adding commonjs support to requirejs isn't bad at all, we can either use write a custom plugin that does it automagically, or use a premade one -> https://github.com/guybedford/cjs . The premade one breaks backwards compatibility, requiring people to use the following syntax in their requirejs paths cjs!path/to/file.

@rgbkrk
Copy link
Member

rgbkrk commented Aug 13, 2015

Oh yuck to that cjs one. Perhaps the terrible question from me here is if we can introduce the packages we've been making as AMD modules for the notebook.

@Carreau
Copy link
Member

Carreau commented Aug 14, 2015

As long as the dev cycle is fast and easy to set-up I'm ok with that.
If you can just write a quick wrap up on how to test that, I'm can give it a try.

@ellisonbg
Copy link
Contributor

Kyle - can you clarify - I thought you wanted us to move away from AMD
modules?

On Thu, Aug 13, 2015 at 5:55 PM, Matthias Bussonnier <
notifications@github.com> wrote:

As long as the dev cycle is fast and easy to set-up I'm ok with that.
If you can just write a quick wrap up on how to test that, I'm can give it
a try.


Reply to this email directly or view it on GitHub
#268 (comment).

Brian E. Granger
Cal Poly State University, San Luis Obispo
@ellisonbg on Twitter and GitHub
bgranger@calpoly.edu and ellisonbg@gmail.com

@Carreau
Copy link
Member

Carreau commented Aug 14, 2015

Kyle is speaking of transpiling to AMD.

Write as common JS, and make them AMD loadable.

@rgbkrk
Copy link
Member

rgbkrk commented Aug 14, 2015

I'm trying to compromise on what works for the current notebook and widgets which are wedded to requirejs.

I'll be checking this PR out again tomorrow.

@rgbkrk
Copy link
Member

rgbkrk commented Aug 14, 2015

As in I'm definitely willing to figure out what needs to be done to make some kind of transition to working in the current notebook.

Our new packages should all be commonjs. What I'm saying here is that maybe there's an extra step (as part of notebook js build) to convert to AMD or otherwise adapt.

@ellisonbg
Copy link
Contributor

Also maybe try the nbgrader extension?

On Wed, Aug 26, 2015 at 2:45 PM, Kyle Kelley notifications@github.com
wrote:

Merged #268 #268.


Reply to this email directly or view it on GitHub
#268 (comment).

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

@Carreau
Copy link
Member

Carreau commented Aug 27, 2015

Please dont' merge such huge things on a rush.
You just broke everything.

#353

@Carreau Carreau mentioned this pull request Aug 27, 2015
@Carreau
Copy link
Member

Carreau commented Aug 27, 2015

Just again expressing my frustration that now most of the PR that were ok to merge need to be rebase, and has now we need to discuss wether this was or not 4.1 material or we need to revert, I basically cannot do a thing, as otherwise I'll need to rebase again.

I cannot either tell user to rebase theirs as if I tell that, they might have to de-rebase if we revert.

😡 :rage4:

We usually warn 24 to 48h in advance on ML before doing such big change.

Nevertheless, despite the rush, and wether we roll back or not I'm happy to see that working.
But I'm still unhappy. Great job still 🍻 . But unhappy. Grrrrr.

@parente
Copy link
Member

parente commented Aug 27, 2015

@rgbkrk Alright, you're going to hate me Jon but are there any common notebook extensions I should test with this PR, to confirm this can be part of a 4.x release?

I know this work started before we had the conversation about declarative widgets and dashboards which we've implemented as extensions. I suspect this MR breaks what we've done.

Two things that would help us as we start to integrate better with the community would be:

  1. Where do we look to find out about these big changes that are about to go in?
  2. What's considered stable API for extensions to touch, if anything? For example, right now our work uses requirejs to load JS. I see this commit retains it for compatibility. But is it going to disappear eventually? And if so what should extensions (which need to load dynamically by definition) do instead?

@Carreau
Copy link
Member

Carreau commented Aug 27, 2015

@parente we would usually not do that this way see this this mail I sent a few hours after: https://groups.google.com/forum/#!topic/jupyter/lkK9rOPEYMc

@ellisonbg
Copy link
Contributor

Peter, sorry that this dropped with little advanced notice.

  • We will likely need to have something like requirejs for loading assets
    dynamically - we are not quite sure eactly how it work work, but there will
    definitely be something like requirejs, if not requirejs itself in the
    notebook.
  • In terms of the JS API stability, we have always warned (perhaps not
    visibly enough) that the JS is not at all stable. The refactoring that iis
    starting to happen will definitely break the APIs in significant ways. Our
    hope though is that the new APIs will be declared stable and will also be
    much easier to work with for extension developers.
  • In terms of communication, the recent split into more repos and the
    increase in contributions is showing the weaknesses in our communications
    models. Fernando and I had a good talk about that last night and will be
    proposing some ideas to the community and steering council to address these
    challenges.

On Thu, Aug 27, 2015 at 6:54 AM, Peter Parente notifications@github.com
wrote:

@rgbkrk https://github.com/rgbkrk Alright, you're going to hate me Jon
but are there any common notebook extensions I should test with this PR, to
confirm this can be part of a 4.x release?

I know this work started before we had the conversation about declarative
widgets and dashboards which we've implemented as extensions. I suspect
this MR breaks what we've done.

Two things that would help us as we start to integrate better with the
community would be:

  1. Where do we look to find out about these big changes that are about
    to go in?
  2. What's considered stable API for extensions to touch, if anything?
    For example, right now our work uses requirejs to load JS. I see this
    commit retains it for compatibility. But is it going to disappear
    eventually? And if so what should extensions (which need to load
    dynamically by definition) do instead?


Reply to this email directly or view it on GitHub
#268 (comment).

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

@ellisonbg
Copy link
Contributor

Hi, Matthias sorry this went in without getting your input :(

I think there are few aspects of this:

Communication:

This is another example of how our communication model is straining under
the many repos and increased number of contributors. I am encountering this
type of communications failure in many corners of the project and we need
to do something about it - will post to the main jupyter list about this in
a few minutes. You are not the only person who is finding yourself in this
situation (myself included).

Merging fast:

For the last few weeks, there has been a "merge fast and iterate" effort in
this particular repo. This is in response to our not wanting development of
the notebook to stall while we do refactoring. Other PRs that were merged
fast were the multi-cell selection, command pallette and search+replace (I
personally am fine with this approach). I think the rest of the team sensed
that the door was open for the "merge fast and iterate" approach. With that
said, there is no reason we couldn't have waiting longer (see communication
above).

Technical details of this PR:

There is broad agreement that we are moving torwards having our JS in
separate repos/packages that are distributed using npm. Right now we have
quite a few developers who are building those new npm packges
(jupyter-js-services, jupyter-js-output). The work of starting to use all
of those new frontend npm packages is bottlenecking on this PR. Because of
that I think it is 1) important to merge this quickly and 2) release it in
4.1 so we can have a single notebook effort moving forward that has both
new features for our users and new refactored code underneath.

On Thu, Aug 27, 2015 at 1:09 AM, Matthias Bussonnier <
notifications@github.com> wrote:

Just again expressing my frustration that now most of the PR that were ok
to merge need to be rebase, and has now we need to discuss wether this was
or not 4.1 material or we need to revert, I basically cannot do a thing, as
otherwise I'll need to rebase again.

I cannot either tell user to rebase theirs as if I tell that, they might
have to de-rebase if we revert.

[image: 😡] [image: :rage4:]

We usually warn 24 to 48h in advance on ML before doing such big change.

Nevertheless, despite the rush, and wether we roll back or not I'm happy
to see that working.
But I'm still unhappy. Great job still [image: 🍻] . But unhappy.
Grrrrr.


Reply to this email directly or view it on GitHub
#268 (comment).

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

@rgbkrk
Copy link
Member

rgbkrk commented Aug 27, 2015

When @Carreau and I met this morning, his biggest concern was that we should probably not make this part of 4.x since we're not sure how stable it is and there are bug fixes slated for 4.1. We could create a 4.x branch that predates this PR, and move forward with this as 4.2 or 5.x.

@rgbkrk
Copy link
Member

rgbkrk commented Aug 27, 2015

I'm certainly happy to revert, apply @Carreau's patch that fixes the command palette/search (perhaps we need tests there), and then re-open it for further discussion.

@Carreau
Copy link
Member

Carreau commented Aug 27, 2015

Merging fast:

The other PRs where purely addition and self contained. This is actually a gigantic change that break some (fixable) things, and might also have broken many other things.
So it is not the same.

Technical details of this PR:

I'm fine with this PR, but we need a stable 4.1 have seen how many issues we have with 4.0 ? We can't afford to get a 4.1 which is more work to package than 4.0 and have downstream not upgrading from 4.0 to 4.1 because it breaks things. The rule as always been no refactor or big changes for 4.1, then we fork-of a dev branch and backport.

Let's wait for US to finish waking up and respond on ML. We can even wait a few days, but right now, I would be a maintainer of downstream project I would just be pissed of for breaking the meaning of semantic versioning if this is a 4.1

@ellisonbg
Copy link
Contributor

Yes, we may want to move this a branch and wait until after 4.1.

At the same time, there is an elephant in the room: our JavaScript APIs are
not stable, have never been stable, and we are about to break all of them
completely. After that, they will be stable in the versions/semver sense.

Here is my preference: because our JS APIs are not public, we are free to
break them in each of the 4.x releases. It is my understanding that in
semver, internal refactoring is fine across point releases.

The alternative is to do refactoring in a branch until 5.0, which I don't
feel is reasonable if master evolves quickly at the same time. I don't
think we would ever be able to merge the two.

On Thu, Aug 27, 2015 at 7:33 AM, Matthias Bussonnier <
notifications@github.com> wrote:

Merging fast:

The other PRs where purely addition and self contained. This is actually a
gigantic change that break some (fixable) things, and might also have
broken many other things.
So it is not the same.

Technical details of this PR:

I'm fine with this PR, but we need a stable 4.1 have seen how many issues
we have with 4.0 ? We can't afford to get a 4.1 which is more work to
package than 4.0 and have downstream not upgrading from 4.0 to 4.1 because
it breaks things. The rule as always been no refactor or big changes
for 4.1, then we fork-of a dev branch and backport.

Let's wait for US to finish waking up and respond on ML. We can even wait
a few days, but right now, I would be a maintainer of downstream project I
would just be pissed of for breaking the meaning of semantic versioning if
this is a 4.1


Reply to this email directly or view it on GitHub
#268 (comment).

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

@rgbkrk
Copy link
Member

rgbkrk commented Aug 27, 2015

Ping @jasongrout

@rgbkrk
Copy link
Member

rgbkrk commented Aug 27, 2015

If we keep this, we can start writing unit tests with mocha directly right now (though we could have with requirejs too).

@ellisonbg
Copy link
Contributor

I like that !

On Thu, Aug 27, 2015 at 11:55 AM, Kyle Kelley notifications@github.com
wrote:

If we keep this, we can start writing unit tests with mocha directly right
now (though we could have with requirejs too).


Reply to this email directly or view it on GitHub
#268 (comment).

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

@Carreau
Copy link
Member

Carreau commented Aug 28, 2015

Ok, one other negative that I didn't saw. Moving everything to static-src break the history where all file looks in github like having only 1 commit from Jon.

@rgbkrk
Copy link
Member

rgbkrk commented Aug 28, 2015

Well that's no good.

@rgbkrk
Copy link
Member

rgbkrk commented Aug 28, 2015

I'm currently feeling like we should revert and that we can continue discussions about the PR itself, same as seeing what can be broken out into smaller PRs.

@ellisonbg
Copy link
Contributor

Is there a way of implementing this stuff without collapsing the git
history?

On Fri, Aug 28, 2015 at 4:42 AM, Kyle Kelley notifications@github.com
wrote:

I'm currently feeling like we should revert and that we can continue
discussions about the PR itself, same as seeing what can be broken out into
smaller PRs.


Reply to this email directly or view it on GitHub
#268 (comment).

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

@Carreau
Copy link
Member

Carreau commented Aug 28, 2015

The git history is ok, it just confuse most of the tools around because the
all tree is moved/file renamed.
We can just not move things all around.

On Fri, Aug 28, 2015 at 4:21 PM, Brian E. Granger notifications@github.com
wrote:

Is there a way of implementing this stuff without collapsing the git
history?

On Fri, Aug 28, 2015 at 4:42 AM, Kyle Kelley notifications@github.com
wrote:

I'm currently feeling like we should revert and that we can continue
discussions about the PR itself, same as seeing what can be broken out
into
smaller PRs.


Reply to this email directly or view it on GitHub
#268 (comment).

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


Reply to this email directly or view it on GitHub
#268 (comment).

@ellisonbg
Copy link
Contributor

I guess that is what I am asking. Sometimes we need to move things around
to refactor. Can we do that while addressing the version control related
things you are bringing up?

On Fri, Aug 28, 2015 at 8:27 AM, Matthias Bussonnier <
notifications@github.com> wrote:

The git history is ok, it just confuse most of the tools around because the
all tree is moved/file renamed.
We can just not move things all around.

On Fri, Aug 28, 2015 at 4:21 PM, Brian E. Granger <
notifications@github.com>
wrote:

Is there a way of implementing this stuff without collapsing the git
history?

On Fri, Aug 28, 2015 at 4:42 AM, Kyle Kelley notifications@github.com
wrote:

I'm currently feeling like we should revert and that we can continue
discussions about the PR itself, same as seeing what can be broken out
into
smaller PRs.


Reply to this email directly or view it on GitHub
#268 (comment).

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


Reply to this email directly or view it on GitHub
#268 (comment).


Reply to this email directly or view it on GitHub
#268 (comment).

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

@jdfreder
Copy link
Contributor Author

Because the files are being transpiled from one context into another, and they reference each other, sometimes with abs path, they need to live in two separate parent directories. This means the .js files move from static/ to somewhere, or the .less & .css move from /static to somewhere and somewhere becomes the new static directory. I just picked the prior because it made more sense. I tried to move everything with git mv, I'm not sure why some of the files appear as new.

@ellisonbg
Copy link
Contributor

Ahh, Ok so the files moving are not API breakages, but they led to files
having a somewhat confusing git record.

On Fri, Aug 28, 2015 at 5:36 PM, Jonathan Frederic <notifications@github.com

wrote:

Because the files are being transpiled from one context into another, and
they reference each other, sometimes with abs path, they need to live in
two separate parent directories. This means the .js files move from static/
to somewhere, or the .less & .css move from /static to somewhere and
somewhere becomes the new static directory. I just picked the prior because
it made more sense. I tried to move everything with git mv, I'm not sure
why some of the files appear as new.


Reply to this email directly or view it on GitHub
#268 (comment).

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

@minrk minrk modified the milestone: 4.1 Dec 1, 2015
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 24, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants