Xterm.js: Browserified search addon includes a copy of Terminal

[Tyriar]

Regressed in https://github.com/sourcelair/xterm.js/pull/834

When browserify sees this:

module.exports = addon(require('../../xterm'));

It merges all of xterm into this file. One way to work around this is to pull the string out so browserify doesn't pick it up:

const xterm = '../../xterm';
module.exports = addon(require(xterm));

This produces a warning in webpack:

WARNING in ../~/xterm/lib/addons/search/search.js
10:31-45 Critical dependency: the request of a dependency is an expression

This warning is preferable really, I wonder if there is a better way to build something (search addon) that depends on a lib (xterm.js) without packaging the lib with it.

[Tyriar]

Possible fix is for instead of search.ts to find Terminal.ts, have Terminal.loadAddon load the addon and then pass Terminal into it.

In search.ts:

    /**
     * CommonJS environment
     */
    module.exports = addon;

In Terminal.ts:

public static loadAddon(addon: string, callback?: Function): boolean | any {
    // TODO: Improve return type and documentation
    if (typeof exports === 'object' && typeof module === 'object') {
      // CommonJS
      return require('./addons/' + addon + '/' + addon)(Terminal);
    } else if (typeof define === 'function') {
...

[mofux]

I like your proposal. The addon shouldn't have to require the terminal as the terminal already requires the addon, which feels strange. Passing the terminal prototype to the addon when it gets loaded feels much more consistent to me. This also brings benefits for testing, because we can pass a mock terminal to the addon.

[parisk]

Yep, I agree too. The terminal can pass its prototype to the add-on.

We will have to document though what happens with add-ons that are being loaded without a module system (e.g. just by using script tags). I guess that the user will just have to initialize the add-on on their own.

[Tyriar]

Well we can continue to keep using window.Terminal for the web case. As for requirejs, marking the terminal module as a dependency and keeping the current structure should be fine?

[Tyriar]

I'm consolidating all bundling/loadAddon related issues into https://github.com/sourcelair/xterm.js/issues/1018, please comment on that issue if you have thoughts :smiley: