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

Add Session class and Session options decoration by plugins #3464

Merged
merged 3 commits into from
Feb 10, 2019

Conversation

chabou
Copy link
Collaborator

@chabou chabou commented Feb 6, 2019

This PR implements this RFC

It adds 2 new plugin hook to our API decorateSessionClass and decorateSessionOptions.

It unlocks some awesome new plugin possiblities. I made a plugin as an example (and a proof of concept): https://github.com/chabou/hyper-session-log

@chabou chabou requested a review from juancampa February 6, 2019 22:46
app/plugins.js Show resolved Hide resolved
Copy link
Contributor

@juancampa juancampa left a comment

Choose a reason for hiding this comment

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

Just one question/change. Otherwise LGTM

app/session.js Outdated
@@ -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
Collaborator 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.

@@ -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! 🎉

@juancampa
Copy link
Contributor

juancampa commented Feb 10, 2019

Thanks for addressing my feedback @chabou! Also, thanks for being patience with my response delay, I'm in the process of moving to a new place and these days have been pretty hectic 😅

I think it's reasonable to expect that a plugin that overrides init to prevent pty creation should also override other methods that use the pty (such as write). It might be better to have the base class be consistent and allow any potential inconsistencies to be handled by plugins that introduce them.

This approach could cause problems if, for example, there's a plugin that overrides init to do something else but still wants the pty to be created. It won't see any errors when write is called because it "accidentally" removed the pty

@juancampa
Copy link
Contributor

In short: if a plugin wants to remove the pty, it should be its responsibility to also override the other methods.

@chabou
Copy link
Collaborator Author

chabou commented Feb 10, 2019

Look at destroy() method.
I don't want to force plugin to override it only to prevent an error with pty.destroy().

My point is that plugin should focus on what it adds and not what it removes. Having base class that have conditional behavior, anticipating that some part could be removed is a good thing.

But I totally understand your point that missing pty could be an unwanted error and silent protection could be harmful.

I will add some warning.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants