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

Upgrade xterm to 3.11.0 and remove vendored addon #787

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions binderhub/app.py
Original file line number Diff line number Diff line change
Expand Up @@ -444,6 +444,9 @@ def initialize(self, *args, **kwargs):
kubernetes.config.load_kube_config()
self.tornado_settings["kubernetes_client"] = self.kube_client = kubernetes.client.CoreV1Api()

else:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this change related to the upgrade?

Copy link
Member Author

Choose a reason for hiding this comment

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

I needed this (as a hack) to be able to run in "frontend dev only mode". I didn't spend a lot of time trying to work out why this change or how else one could fix it. Once this PR works we should think about the proper way of fixing this (or punting to a new PR).

self.tornado_settings["kubernetes_client"] = None


# times 2 for log + build threads
self.build_pool = ThreadPoolExecutor(self.concurrent_build_limit * 2)
Expand Down
19 changes: 10 additions & 9 deletions binderhub/static/js/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,10 @@
pushing -> built
pushing -> failed
*/
import * as Terminal from 'xterm';
import { Terminal } from 'xterm';
import { fit } from 'xterm/lib/addons/fit/fit';
//Terminal.applyAddon(fit);

import Clipboard from 'clipboard';
import 'xterm/lib/xterm.css';
import 'bootstrap';
Expand All @@ -24,10 +27,6 @@ import 'bootstrap/dist/css/bootstrap.min.css';
import 'bootstrap/dist/css/bootstrap-theme.min.css';
import '../index.css';

// FIXME: Can not seem to import this addon from npm
// See https://github.com/xtermjs/xterm.js/issues/1018 for more details
import {fit} from './vendor/xterm/addons/fit';

var BASE_URL = $('#base-url').data().url;

function update_favicon(path) {
Expand Down Expand Up @@ -144,7 +143,7 @@ function build(providerSpec, log, path, pathType) {
image.onStateChange('*', function(oldState, newState, data) {
if (data.message !== undefined) {
log.write(data.message);
log.fit();
fit(log); //log.fit();
Copy link
Member Author

Choose a reason for hiding this comment

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

Turning this around so that you don't need applyAddon is what jupyter lab does as well and seems to work.

} else {
console.log(data);
}
Expand Down Expand Up @@ -205,11 +204,11 @@ function setUpLog() {
disableStdin: true
});

log.open(document.getElementById('log'), false);
log.fit();
log.open(document.getElementById('log'));
fit(log); //log.fit();

$(window).resize(function() {
log.fit();
fit(log); //log.fit();
});

var $panelBody = $("div.panel-body");
Expand All @@ -226,8 +225,10 @@ function setUpLog() {
log.toggle = function () {
if ($panelBody.hasClass('hidden')) {
log.show();
fit(log);
} else {
log.hide();
fit(log);
}
};

Expand Down
86 changes: 0 additions & 86 deletions binderhub/static/js/vendor/xterm/addons/fit.js

This file was deleted.

2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
"jquery": "^3.2.1",
"style-loader": "^0.19.1",
"webpack": "^3.10.0",
"xterm": "^2.9.2"
"xterm": "^3.11.0"
},
"scripts": {
"webpack": "webpack",
Expand Down