Skip to content

Add Session class and Session options decoration by plugins #3464

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Feb 10, 2019
Merged

Add Session class and Session options decoration by plugins #3464

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c727b1b
Add Session class and options plugin decoration
chabou Feb 6, 2019
789b8fb
add init() method
chabou Feb 9, 2019
2937717
Add warning for non-overrided session methods
chabou Feb 10, 2019
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
22 changes: 19 additions & 3 deletions app/plugins.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -332,9 +332,9 @@ exports.onWindow = win => {
});
};

// decorates the base object by calling plugin[key]
// decorates the base entity by calling plugin[key]
// for all the available plugins
function decorateObject(base, key) {
function decorateEntity(base, key, type) {
let decorated = base;
modules.forEach(plugin => {
if (plugin[key]) {
Expand All @@ -345,7 +345,7 @@ function decorateObject(base, key) {
notify('Plugin error!', `"${plugin._name}" when decorating ${key}`, {error: e});
return;
}
if (res && typeof res === 'object') {
if (res && (!type || typeof res === type)) {
decorated = res;
} else {
notify('Plugin error!', `"${plugin._name}": invalid return type for \`${key}\``);
Expand All @@ -356,6 +356,14 @@ function decorateObject(base, key) {
return decorated;
}

function decorateObject(base, key) {
return decorateEntity(base, key, 'object');
}

function decorateClass(base, key) {
return decorateEntity(base, key, 'function');
}

exports.getDeprecatedConfig = () => {
const deprecated = {};
const baseConfig = config.getConfig();
Expand Down Expand Up @@ -409,4 +417,12 @@ exports.getDecoratedBrowserOptions = defaults => {
return decorateObject(defaults, 'decorateBrowserOptions');
};

exports.decorateSessionClass = Session => {
return decorateClass(Session, 'decorateSessionClass');
};

exports.decorateSessionOptions = defaults => {
return decorateObject(defaults, 'decorateSessionOptions');
};

exports._toDependencies = toDependencies;
2 changes: 2 additions & 0 deletions app/plugins/extensions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ module.exports = {
'onWindow',
'onRendererWindow',
'onUnload',
'decorateSessionClass',
'decorateSessionOptions',
'middleware',
'reduceUI',
'reduceSessions',
Expand Down
6 changes: 3 additions & 3 deletions app/session.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,12 +141,12 @@ module.exports = class Session extends EventEmitter {
}

write(data) {
this.pty.write(data);
this.pty && this.pty.write(data);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
this.pty && this.pty.write(data);
if (!this.pty) {
console.error('Attempted to write to a session with no pty');
}
this.pty.write(data);

Maybe this? Looks like otherwise we could silently lose data. Or is there a case where this would be valid?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I anticipated the case: A plugin overrides the constructor and prevents the pty creation (without override this write method). It seems legit to fail silently.

But thanks to your comment, I think this is currently pretty hard to prevent this pty creation because it occurs in constructor. Such a plugin can't call super() and would break other plugins that have previously decorated this Class.

I think we should isolate pty creation in a init(options) function that plugin would override to prevent this pty creation without breaking constructor chain.

}

resize({cols, rows}) {
try {
this.pty.resize(cols, rows);
this.pty && this.pty.resize(cols, rows);
} catch (err) {
//eslint-disable-next-line no-console
console.error(err.stack);
Expand All @@ -155,7 +155,7 @@ module.exports = class Session extends EventEmitter {

destroy() {
try {
this.pty.kill();
this.pty && this.pty.kill();
} catch (err) {
//eslint-disable-next-line no-console
console.error('exit error', err.stack);
Expand Down
8 changes: 5 additions & 3 deletions app/ui/window.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ const Session = require('../session');
const contextMenuTemplate = require('./contextmenu');
const {execCommand} = require('../commands');
const {setRendererType, unsetRendererType} = require('../utils/renderer-utils');
const {decorateSessionOptions, decorateSessionClass} = require('../plugins');

module.exports = class Window {
constructor(options_, cfg, fn) {
Expand Down Expand Up @@ -89,7 +90,7 @@ module.exports = class Window {
function createSession(extraOptions = {}) {
const uid = uuid.v4();

const options = Object.assign(
const defaultOptions = Object.assign(
{
rows: 40,
cols: 100,
Expand All @@ -101,8 +102,9 @@ module.exports = class Window {
extraOptions,
{uid}
);

const session = new Session(options);
const options = decorateSessionOptions(defaultOptions);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is cool, more extensibility! 🎉

const DecoratedSession = decorateSessionClass(Session);
const session = new DecoratedSession(options);
sessions.set(uid, session);
return {session, options};
}
Expand Down