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

Should .dispose() be a trapdoor function? #3939

Closed
xobs opened this issue Jul 26, 2022 · 3 comments · Fixed by #3958
Closed

Should .dispose() be a trapdoor function? #3939

xobs opened this issue Jul 26, 2022 · 3 comments · Fixed by #3958
Assignees
Labels
type/documentation Regarding website, API documentation, README, etc.
Milestone

Comments

@xobs
Copy link

xobs commented Jul 26, 2022

As part of lifecycle management, xterm.dispose() exists to dispose of a terminal object. The documentation states that this function:

    /*
     * Disposes of the terminal, detaching it from the DOM and removing any
     * active listeners.
     */
    dispose(): void;

However, as part of this process, it also overwrites the .write() call, making the terminal no longer usable.

It seems as though the opposite would be open():

    /**
     * Opens the terminal within an element.
     * @param parent The element to create the terminal within. This element
     * must be visible (have dimensions) when `open` is called as several DOM-
     * based measurements need to be performed when this function is called.
     */
    open(parent: HTMLElement): void;

Should this be the case? Should .open() reinstate the .write() call? Or should the documentation be updated to reflect the fact that this is an error, and perhaps a warning be added to .write() or .open() to indicate the user is using a terminal that has been disposed of?

This is an extension of the discussion from #3910

@Tyriar
Copy link
Member

Tyriar commented Jul 26, 2022

When you dispose the object it's not meant to be used again, you may be able to but thing will inevitably go wrong. For clear the write call, that's just always been like that since the project was created. We wouldn't normally clear a method like that though.

For robert-harbison/xterm-for-react#13 it definitely shouldn't be using dispose, instead I think the best way now is to remove the container element (that was opened) from the DOM, and then when it's added, call open again.

@xobs
Copy link
Author

xobs commented Jul 26, 2022

Thanks for the explanation -- it seems like dispose() is effectively a destructor. Perhaps that terminology is common in Javascript but I'm just not familiar with it.

Would it make sense to add a sentence to the dispose() API description stating that the object should never be used after it is disposed of?

@Tyriar
Copy link
Member

Tyriar commented Jul 26, 2022

It's used throughout vscode and xterm.js but yes it's never made super clear. I think it's a good idea to change the description of Terminal.dispose to make it clear👍

@Tyriar Tyriar added this to the 5.0.0 milestone Jul 28, 2022
@Tyriar Tyriar self-assigned this Jul 28, 2022
@Tyriar Tyriar added the type/documentation Regarding website, API documentation, README, etc. label Jul 28, 2022
Tyriar added a commit to Tyriar/xterm.js that referenced this issue Jul 28, 2022
Tyriar added a commit to Tyriar/xterm.js that referenced this issue Jul 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/documentation Regarding website, API documentation, README, etc.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants