Addon that refreshes xterm upon webfont load #1368

vincentwoo · 2018-04-04T02:10:49Z

Addresses #1164

I'm new to TypeScript, so I'm not sure if I've done everything the right way. I've opted to monkeypatch _setup, because we need to get into the creation flow as soon as possible (and also because only _setup references what the default font options are). It's possible to do something like require a manual call to loadWebfonts but I've opted for a more seamless approach.

The addon also doesn't add any more configuration - it assumes that any font families passed in will be webfonts. If they aren't, there may be a momentary flicker but all should continue to work.

Tyriar · 2018-04-04T16:54:45Z

src/addons/webfont/webfont.ts

+    const regular = new FontFaceObserver(originalFont).load();
+    const bold = new FontFaceObserver(originalFont, { weight: 'bold' }).load();
+
+    regular.constructor.all([regular, bold]).then(() => {


Is this the same as Promise.all?

Yes, but without relying on importing a specific Promise library.

Tyriar · 2018-04-04T16:57:00Z

package.json

@@ -88,5 +88,7 @@
    "webpack": "gulp webpack",
    "watch": "gulp watch"
  },
-  "dependencies": {}
+  "dependencies": {
+    "fontfaceobserver": "*"


I prefer the approach taken with zmodem here; leave it up to the consumer to include the dependency (we should add a README to the addon dir to call that out though).

xterm.js/src/addons/zmodem/zmodem.ts

Line 88 in 989f587

zmodem = (typeof window === 'object') ? (<any>window).ZModem : {Browser: null}; // Nullify browser for tests

doable. what should behavior be if you try to run the addon and the dependency is not present?

I guess console.warn and open normally?

Tyriar · 2018-04-04T17:04:53Z

src/addons/webfont/webfont.ts

+  (<any>terminalConstructor.prototype)._setup = function (): void {
+    const originalFont = this.options.fontFamily;
+    if (loadedFonts[originalFont]) return oldSetup.call(this);
+    delete this.options.fontFamily;


A more explicit loadWebfonts.load(fontFamily: string) would be preferable imo and align better with how the other addons work.

Also the font is only ever used after open is called. So what if this function was shaped more like this, rather than monkeypatching?

loadAndOpen = function (fontFamily: string, element: HTMLElement): void { Promise.all(...).then(() => { this.setOption('fontFamily', fontFamily); this.open(element) }); };

One rationale for doing it on _setup is that you may create a term, write to it, and then later render it. If we know you're going to need a webfont, we can get to it before open is actually called and avoid the resize flash.

If that's not as valuable to you as having a less magical thing, I can go ahead and change the surface.

Also, if we're going to call the endpoint "load and open" we might as well just delay opening the terminal until fonts come in to avoid the reflash as well.

Also, if we're going to call the endpoint "load and open" we might as well just delay opening the terminal until fonts come in to avoid the reflash as well.

That's what my proposal does, it doesn't touch the DOM until the font load promises are finished?

I'm down. Hang tight.

vincentwoo · 2018-04-07T00:02:53Z

Okay, I've updated the PR. Two things:

I've made the surface loadWebfontAndOpen = function (element: HTMLElement), opting not to pass the font as a separate option to the addon. I think it's a bit dangerous for users to potentially specify font in two places.
I want to push for a better package story than "require the user add dependencies to the global window object". Can we add a dependency to just the addon somehow? I want a user's build system to simply import the addon and have the right thing happen. Usability of addons go down if you require the user to manually pollute the global object just to use them. Complicating this, FontFaceObserver is not fully ES6 yet and does require a require to work.

vincentwoo · 2018-04-10T21:49:20Z

@Tyriar get a chance to see this?

Tyriar · 2018-04-11T18:26:31Z

I want to push for a better package story than "require the user add dependencies to the global window object". Can we add a dependency to just the addon somehow? I want a user's build system to simply import the addon and have the right thing happen. Usability of addons go down if you require the user to manually pollute the global object just to use them. Complicating this, FontFaceObserver is not fully ES6 yet and does require a require to work.

I don't want consumers who don't need zmodem or fontfaceobserver to need to download them when installing the package and also to remove them manually when they don't need them.

How about you pass it into the function then? That seems much better than relying on a global:

loadWebfontAndOpen = function (element: HTMLElement, ffo: FontFaceObserver)

webfont.loadWebfontAndOpen(element, require('fontfaceobserver'));

vincentwoo · 2018-04-11T19:22:03Z

Would it be better to just make this addon a separate NPM package?

…

On Wed, Apr 11, 2018 at 11:26 AM, Daniel Imms ***@***.***> wrote: I want to push for a better package story than "require the user add dependencies to the global window object". Can we add a dependency to just the addon somehow? I want a user's build system to simply import the addon and have the right thing happen. Usability of addons go down if you require the user to manually pollute the global object just to use them. Complicating this, FontFaceObserver is not fully ES6 yet and does require a require to work. I don't want consumers who don't need zmodem or fontfaceobserver to need to download them when installing the package and also to remove them manually when they don't need them. How about you pass it into the function then? That seems much better than relying on a global: loadWebfontAndOpen = function (element: HTMLElement, ffo: FontFaceObserver) webfont.loadWebfontAndOpen(element, require('fontfaceobserver')); — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1368 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAlbyFCbeDV7c5oDnD9-Qe9B6A6O6cKgks5tnkrZgaJpZM4TGD1r> .

Tyriar · 2018-04-11T19:53:03Z

That's the hope eventually (I just commented about this #1128 (comment)), in which can it could be moved to a regular dependency.

vincentwoo · 2018-04-11T20:45:24Z

I can kill this PR and just release this on my own if that's what you prefer.

…

On Wed, Apr 11, 2018 at 12:53 PM, Daniel Imms ***@***.***> wrote: That's the hope eventually (I just commented about this #1128 (comment) <#1128 (comment)>), in which can it could be moved to a regular dependency. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1368 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAlbyKWRecOlPnqujIMUhwCxmNHBeiz2ks5tnl8hgaJpZM4TGD1r> .

Tyriar · 2018-04-11T21:49:06Z

Discoverability is then the main problem with third party addons at the moment as we don't really have a good story for that. It's up to you whether you want to do that or merge it in. Eventually this would be best to live as a third party addon not maintained by the team though.

mofux · 2018-04-11T21:52:11Z

Couldn't we simply add a section to our README that lists 3rd Party Addons, similar to the Real-world uses?

Tyriar · 2018-04-12T02:51:29Z

@mofux we could, I don't think it's a long term solution though. The readme is very long already.

vincentwoo · 2018-04-16T11:06:43Z

I've split out this PR as its own package: https://www.npmjs.com/package/xterm-webfont. I'll leave it up to you on whether to link to it somewhere or not. I think this is a good middle ground and long-term place to be wrt addons.

Tyriar · 2018-04-16T13:29:21Z

#1390

Tyriar requested changes Apr 4, 2018

View reviewed changes

Tyriar added this to the 3.4.0 milestone Apr 4, 2018

Addon that refreshes xterm upon webfont load

b6ba743

vincentwoo force-pushed the master branch from 0c8c85e to b6ba743 Compare April 6, 2018 23:58

Return promise that returns xterm instance from addon

0e65a36

Tyriar closed this Apr 16, 2018

Tyriar removed this from the 3.4.0 milestone May 17, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Addon that refreshes xterm upon webfont load #1368

Addon that refreshes xterm upon webfont load #1368

vincentwoo commented Apr 4, 2018 •

edited

Loading

Tyriar Apr 4, 2018

vincentwoo Apr 4, 2018

Tyriar Apr 4, 2018

vincentwoo Apr 4, 2018

Tyriar Apr 4, 2018

Tyriar Apr 4, 2018 •

edited

Loading

vincentwoo Apr 4, 2018

vincentwoo Apr 4, 2018

Tyriar Apr 5, 2018

vincentwoo Apr 5, 2018

vincentwoo commented Apr 7, 2018 •

edited

Loading

vincentwoo commented Apr 10, 2018

Tyriar commented Apr 11, 2018

vincentwoo commented Apr 11, 2018 via email

Tyriar commented Apr 11, 2018

vincentwoo commented Apr 11, 2018 via email

Tyriar commented Apr 11, 2018

mofux commented Apr 11, 2018

Tyriar commented Apr 12, 2018

vincentwoo commented Apr 16, 2018

Tyriar commented Apr 16, 2018

Addon that refreshes xterm upon webfont load #1368

Addon that refreshes xterm upon webfont load #1368

Conversation

vincentwoo commented Apr 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tyriar Apr 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vincentwoo commented Apr 7, 2018 • edited Loading

vincentwoo commented Apr 10, 2018

Tyriar commented Apr 11, 2018

vincentwoo commented Apr 11, 2018 via email

Tyriar commented Apr 11, 2018

vincentwoo commented Apr 11, 2018 via email

Tyriar commented Apr 11, 2018

mofux commented Apr 11, 2018

Tyriar commented Apr 12, 2018

vincentwoo commented Apr 16, 2018

Tyriar commented Apr 16, 2018

vincentwoo commented Apr 4, 2018 •

edited

Loading

Tyriar Apr 4, 2018 •

edited

Loading

vincentwoo commented Apr 7, 2018 •

edited

Loading