Long term planning for the Frontend #774
Comments
|
Fun fact - when I first started writing BinderHub, the very first line of JS I wrote was: It's still there, and the file is over 400lines long now :) |
|
I personally really like TypeScript, and the JupyterLab / nteract communities seem to be using it heavily as well. However, @ellisonbg pointed out it might be overkill for us - and I agree. |
|
My current preference is to start with Option 4, and then see how it goes. |
|
/cc some people who might be interested: @minrk, @betatim, @ellisonbg, @ian-r-rose, @choldgraf, @willingc |
|
Hi @yuvipanda I've often thought it would be nice to have some kind of checkbox on the main page for if you wanted to launch in a JupyterLab/RStudio environment. Rather than selecting URL path and type |
|
IMO option 4 will also make it easier to adopt options 1-3 in the future if we want to move in that direction, I'm +1 on option 4. It feels more like an iterative step rather than a total re-work. put another way: I think that we should do number 4 regardless. The question to me is "should we spend the extra time taking care of 1, 2, or 3 while we do number 4, or should we put off that work to a potential future time" |
|
I'd vote for option 3. The main factor is that it used to be/is possible to use https://github.com/segmentio/create-next-app to get all the webpack/uglify/hot-reload/css shrinking etc stuff "for free". As someone who knows nothing about JS and web dev I enjoyed being taken by the hand and given something that looked useful, worked and wasn't super heavy weight. Since I setup bndr.it it seems things have changed so maybe next.js now has this built in?? https://nextjs.org/learn/ was very useful as a place to look up and (somewhat blindly) follow to get some good practices. I'd split the "more dynamic front page" out from this refactor. Mostly because I think we can get some very easy wins there even if we don't make it dynamic. Having some popular repos shown, "syntactic sugar" like @sgibson91 mentioned, etc could be done already without any JS refactoring. Splitting this out means we could work on it in parallel and maybe pull in a new contributor? |
|
If option 3 is "add no new features, just replicate the current structure of things using a simple framework anybody can use" then I'd be +1 on that. @betatim do you think that a person who has medium experience with HTML but no JS would be able to pick up next.js about as quickly as they'd pick up the current page setup? |
|
For some more shameless lobbying: https://github.com/zeit/next.js/tree/master/examples is a good place to find out how to do stuff. So we can start lightweight but find inspiration and guidance. The recommendation right now is to start with an empty repo, follow nextjs.org/learn and pull stuff from the examples as needed. I think on "do frontend devs use this" next is Ok and not a toolkit that is going away soon. There are also react components we might be able to reuse from the nteract project (@rgbkrk tells me we should check out |
|
I found it easy to build bndr.it with next.js, I find it very hard to understand our JS and DOM stuff on mybinder.org. However I never really got into using jQuery and went straight from writing HTML+CSS 15 years ago to react via a big break of doing "nothing web whatsoever". I'd support the idea (no matter which option) to do the refactor without changing any functionality/looks/design. Then after the switch change design/functions/etc. It splits it into two smaller tasks. Smaller diffs to review, less potential for the refactor to get held up by discussion about changing looks/feel/functionality. |
|
Sounds good - I am just trying to tease apart the level of difficulty that these tools will introduce for people of all technical backgrounds, not just those with web-dev experience. Y'all know more about this than I do, so I trust the community's input on this! |
|
I'm unsure what nextjs gives us over using react, but I guess these are implementation details... I think we should definitely split this into two issues maybe - one for refactoring the current JS, and another for seeing if we should move to something. |
|
nextjs is still react, it mostly just gives you an opinionated scaffolding around the webpack so that you're (hopefully) not having to muck with all the JS config pain. Note: it can still be used with typescript if you want that. |
|
Based on my experience, I think the best flow is Option 4 -> Option 2 -> Option 3. The refactor in Option 4 should split up the contents of the single JS file into different files by UI element. For example, a file for repository input field, the file path input field, etc. Once this refactor is complete, we can go through file by file and convert the set of jQuery events/actions into a React component. We don't have to opt-in to React completely. With the way that React is setup, we can bit by bit render more and more React components on the page as everything is migrated. Once that is done, we can move over to Option 3 to leverage some of the performance improvements that Next.JS gives. IMO, this process allows us to refactor and migrate things bit-by-bit and opt in to the tools as we need them. I'm happy to lead this effort, starting with a lightweight refactor so the site can continue to be deployed while we iterate towards our ultimate end goal. |
|
That sounds like a great way to make progress on this @captainsafia and allow others to help in the process iteratively. Feel free to ping me for review on PRs. |
|
+1 to @captainsafia 's idea, thanks for the input! |
|
<3 @captainsafia would love for you to lead the effort!!! |
|
That sounds like a great plan! Thanks to the js experts. |
A standalone import BinderImage from '@binderhub/client';
var image = new BinderImage();
image.onStateChange('building', () => { ... });Similar to: binderhub/binderhub/static/js/index.js Lines 168 to 225 in 23eeb43 |
|
@jtpio I think there's general agreement that splitting up the mega JS file is a great idea (@captainsafia made some progress on this in #777). That PR has stalled, but perhaps we can look to it for inspiration? Or make some more progress on those items if @captainsafia is still interested? I personally am still very pro-this-idea, we all just have too many other things going on :-) I think the example you give would also be useful for tools like Thebelab to utilize |
|
It looks like #777 was an issue tracking steps to take. Three were done in bite sized chunks, with several available for takers. |
|
@rgbkrk ah you're right, for some reason my brain processed that as a PR 😊 (which makes way more sense... I was wondering why there was a PR that was referencing other bite sized PRs lol). more bite-sized PRs like the ones on that issue to chip away at that would be great! |
|
I'm picking this back up :) I think my first goal is to actually cleanup the |
|
@yuvipanda You may want to try Playwright for testing. Nice ergonomics and fast. |
|
Thanks for the pointer, @willingc! It does look pretty cool! I will pick it up when we get to doing end to end testing. Do you know if it can be used purely for unit testing? From looking at the docs, it looks like jest is still a better fit for unit testing while this is great for end to end. |
Soooo, because `this.state` was initialised to `null` and not 'start', this code was always actually dead. `validateStateTransition` had no branch for `oldState` being null, so always returned `false`. This meant `this.state` was *never* set to current state, and so in all callbacks, `oldState` was always `null`! I validated this again by console.logging `oldState` and `newState` inside `validateStateTransition`. Here's the output for when a full build is performed: ``` null -> waiting null -> fetching null -> unknown null -> building null -> failed null -> built null -> launching ``` And for when launching a previously built image ``` null -> launching null -> ready ``` I also put one just above where `this.state` is set, and that code was never called. I was hoping to remove all this code anyway, as it doesn't actually validate nor test anything - that's really done by unit tests and integration tests. I thought this would hide bugs - but turns out, it was a bug itself, and hid nothing. This also changes the signature of the callback for `onChangeState`, dropping `oldState` and `newState`. `oldState` was always `null`, and in our code, we actually don't use `newState` *at all*. The one place that was conditional on `oldState` being null is now unconditional, because `oldState` was always `null`. Ref jupyterhub#774
JS now also supports writing async code with [Asnc Generators](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/AsyncGenerator) which work very much like how async code would in Python. This allows us to write code that flows better, is more familiar to python folks, and avoids messy callback hell. This code is a *straight* port of the existing callback based code to use an Async Generator instead. The EventIterator library is a very helpful convenience function here, allowing us to translate the callbacks from EventSource into an async generator without having to write a queue of promises ourselves. The iterator is stopped exactly the same way the callbacks were earlier stopped (whenever .stop() is called). All the callbacks have been switched over to a switch / case statement instead. We can get cleaner and use exceptions to handle failures, instead of the 'failure' case, but that can come in a later PR so this is a purely mechanical refactoring from one async method to another. An appropriate test for this is also added. To fully test the EventSource flow, we create a temporary web server that will serve a real captured eventsource response, with 1ms gaps between lines to fully simulate how this would work in reality. This gives us much more confidence about how this would work than mocking. With this, @jupyterhub/binder-client now has 100% unit test coverage! Ref jupyterhub#774
|
I learned about playwright from @bl-aire and we now use it for the browser tests in jupyterhub. It's quite nice. I've since used it to script some tedious tasks I have to do for teaching with a web service that doesn't have an API. I don't think it's really for unittesting, though. But since there's so little that our js does other than talking to BinderHub, maybe full browser tests are all we need? |
|
More useful for component testing. Jest is fine for unit tests. IIRC playwright can execute jest tests too. |
…URLs This is two changes that were easier to make together - BASE_URL and BADGE_BASE_URL are now URL objects rather than strings that are manipulated. With this done, we no longer use string manipulation for URLs anywhere! - Both BASE_URL and BADGE_BASE_URL are now always set, as we had a bunch of code that was using BADGE_BASE_URL if available but falls back to BASE_URL + origin if it was not set. This fallback is now implemented globally - The function to make links and generate badge markup is moved into `@jupyterhub/binderhub-client` as it is reasonably generic and not super specific to our frontend alone. This also involves them not reading BASE_URL and BADGE_BASE_URL globally, but having that information be passed in. Tests are also added here to catch any future issues that may arise. - Note for future fix - BADGE_BASE_URL is really PUBLIC_BASE_URL or similar, as it is used both for the location of the badge image (original intent) but also for the links we generate to share. This is relevant only for federation, where we want shared links to point to mybinder.org even though the API call itself may go to a specific member of the federation. I will do this deprecation + rename in a future PR so as to not make this PR bigger. Ref jupyterhub#774
…URLs This is two changes that were easier to make together - BASE_URL and BADGE_BASE_URL are now URL objects rather than strings that are manipulated. With this done, we no longer use string manipulation for URLs anywhere! - Both BASE_URL and BADGE_BASE_URL are now always set, as we had a bunch of code that was using BADGE_BASE_URL if available but falls back to BASE_URL + origin if it was not set. This fallback is now implemented globally - The function to make links and generate badge markup is moved into `@jupyterhub/binderhub-client` as it is reasonably generic and not super specific to our frontend alone. This also involves them not reading BASE_URL and BADGE_BASE_URL globally, but having that information be passed in. Tests are also added here to catch any future issues that may arise. - Note for future fix - BADGE_BASE_URL is really PUBLIC_BASE_URL or similar, as it is used both for the location of the badge image (original intent) but also for the links we generate to share. This is relevant only for federation, where we want shared links to point to mybinder.org even though the API call itself may go to a specific member of the federation. I will do this deprecation + rename in a future PR so as to not make this PR bigger. Ref jupyterhub#774
…URLs This is two changes that were easier to make together - BASE_URL and BADGE_BASE_URL are now URL objects rather than strings that are manipulated. With this done, we no longer use string manipulation for URLs anywhere! - Both BASE_URL and BADGE_BASE_URL are now always set, as we had a bunch of code that was using BADGE_BASE_URL if available but falls back to BASE_URL + origin if it was not set. This fallback is now implemented globally, and correctly. - BASE_URL is also now always fully qualified, and BADGE_BASE_URL always was. - The function to make links and generate badge markup is moved into `@jupyterhub/binderhub-client` as it is reasonably generic and not super specific to our frontend alone. This also involves them not reading BASE_URL and BADGE_BASE_URL globally, but having that information be passed in. Tests are also added here to catch any future issues that may arise. - Note for future fix - BADGE_BASE_URL is really PUBLIC_BASE_URL or similar, as it is used both for the location of the badge image (original intent) but also for the links we generate to share. This is relevant only for federation, where we want shared links to point to mybinder.org even though the API call itself may go to a specific member of the federation. I will do this deprecation + rename in a future PR so as to not make this PR bigger. Ref jupyterhub#774
…URLs This is two changes that were easier to make together - BASE_URL and BADGE_BASE_URL are now URL objects rather than strings that are manipulated. With this done, we no longer use string manipulation for URLs anywhere! - Both BASE_URL and BADGE_BASE_URL are now always set, as we had a bunch of code that was using BADGE_BASE_URL if available but falls back to BASE_URL + origin if it was not set. This fallback is now implemented globally, and correctly. - BASE_URL is also now always fully qualified, and we document that the python code ensures it has a trailing slash always. - The function to make links and generate badge markup is moved into `@jupyterhub/binderhub-client` as it is reasonably generic and not super specific to our frontend alone. This also involves them not reading BASE_URL and BADGE_BASE_URL globally, but having that information be passed in. Tests are also added here to catch any future issues that may arise. - Note for future fix - BADGE_BASE_URL is really PUBLIC_BASE_URL or similar, as it is used both for the location of the badge image (original intent) but also for the links we generate to share. This is relevant only for federation, where we want shared links to point to mybinder.org even though the API call itself may go to a specific member of the federation. I will do this deprecation + rename in a future PR so as to not make this PR bigger. Ref jupyterhub#774
…URLs This is two changes that were easier to make together - BASE_URL and BADGE_BASE_URL are now URL objects rather than strings that are manipulated. With this done, we no longer use string manipulation for URLs anywhere! - Both BASE_URL and BADGE_BASE_URL are now always set, as we had a bunch of code that was using BADGE_BASE_URL if available but falls back to BASE_URL + origin if it was not set. This fallback is now implemented globally, and correctly. - BASE_URL is also now always fully qualified, and we document that the python code ensures it has a trailing slash always. - The function to make links and generate badge markup is moved into `@jupyterhub/binderhub-client` as it is reasonably generic and not super specific to our frontend alone. This also involves them not reading BASE_URL and BADGE_BASE_URL globally, but having that information be passed in. Tests are also added here to catch any future issues that may arise. - Note for future fix - BADGE_BASE_URL is really PUBLIC_BASE_URL or similar, as it is used both for the location of the badge image (original intent) but also for the links we generate to share. This is relevant only for federation, where we want shared links to point to mybinder.org even though the API call itself may go to a specific member of the federation. I will do this deprecation + rename in a future PR so as to not make this PR bigger. Ref jupyterhub#774
Some were using underscores, which is more python than JS Ref jupyterhub#774
- Optional args get [] around the param name, not the type! - Remove extra increased timeout for some jest tests, as they are not needed - Pass actual URL objects to some of the methods, rather than strings. These accidentally worked but shouldn't be relied upon. - Import just the function used during tests from node modules, rather than the entire module. Ref jupyterhub#774
- Optional args get [] around the param name, not the type! - Remove extra increased timeout for some jest tests, as they are not needed - Pass actual URL objects to some of the methods, rather than strings. These accidentally worked but shouldn't be relied upon. - Import just the function used during tests from node modules, rather than the entire module. - Fix type of `fetch` to indicate that's actually just an *iterator*, not a *generator*. Ref jupyterhub#774
Currently, we have one JS file + 1 HTML file (rendered with Jinja2) that does all the frontend work. This includes:
(2) and (3) are particularly intertwined - they're restyled versions of the same page (HTML + JS).
In the future, I'd like us to improve on the following aspects:
Doing a refactor of the current JS in some form or way while it's still smallish (~400 lines) seems like a good idea to me!
Factors
Some factors to consider:
Options
Here are some options
Option 1: React + TypeScript
We can switch to using TypeScript as a language & use React for frontend components + tests.
Option 2: React + JS
Similar to above, but with just plain JS (JSX) instead of TypeScript.
Option 3: Next.js
Recommended by @betatim: https://nextjs.org/docs
Option 4: No React
We'll refactor and move away from monolithic single JS file, but keep using our current setup of pure DOM manipulation rather than use a library.
There are probably more options than this, but this should be a starting point to discuss!
The text was updated successfully, but these errors were encountered: