Fixes for unified webfonts

[johnspackman]

fixes for webfonts so that the Manifest.json provides.webfonts works; also remove unnecessary third party font resources from the repo; also suppress warnings about .svg files being invalid for non-images

[oetiker]

will this download the fonts at build time ?

[johnspackman]

will this download the fonts at build time ?

no, they're downloaded from a CDN by the client at runtime. If someone wants to self host, then they need to download their own copy and update their Manifest.

I've assumed that this is helpful because we don't necessary want to host other people's fonts (for size, at least) and the CDN version means that we're always up to date, eg the MaterialIcons had slipped to several versions behind

We needed to host those fonts previously because the compiler needed to self-generate the glyph mapping at compile time, but those mappings are precalculated now

[oetiker]

The problem is that most of my customers run a pretty tight ship when it comes to security. Having applications pull in web fonts from remote sources at runtime is a big no no … in this way this will break things again … in Germany there have even been convictions for companies with websites pulling in fonts from Google. Also note that it might be that fonts which address their icons via ligature names might change internal structure which would probably break the existing font maps

[hkollmann]

When I understand @johnspackman correct this is not a problem You can download the fonts manually and change the path in manifest.json. The question is if old projects will work without modifications.

If not we should release it as 8.0-beta version

[zaucker]

When I understand @johnspackman correct this is not a problem You can download the fonts manually and change the path in manifest.json. The question is if old projects will work without modifications.

If not we should release it as 8.0-beta version

If this is correct, then the question is, what the default should be.

[johnspackman]

When I understand @johnspackman correct this is not a problem You can download the fonts manually and change the path in manifest.json

Yes exactly - you can choose to override the font definition with your own and use a self hosted font if that's what you want to do. I've just tested (and, ahem, committed) that is what happens.

Existing apps would continue to work, but the qooxdoo distribution (and the compiled app) would be much smaller.

The size of the fonts led to the issue - they inflated the target directory quite a bit when they were not necessarily used, so the @asset was removed.

a pretty tight ship when it comes to security … in Germany there have even been convictions for companies with websites pulling in fonts from Google

Wow! I can't imagine what Germany has that would make it illegal to use a CDN, but that sounds pretty unusual though?

From our point of view, do we really want to be hosting (and including in our npm releases) every font we might ever want to use? Especially as it makes every application target larger?

[derrell]

I don't think we need to host, in our distribution, every font. Can't we, though (maybe even by default), download at compile time, from CDN, those fonts that are used, and build them into the app? And then make it a compile.json easy change to say, instead of compiling them into the app, to download them from CDN at run-time?

[oetiker]

yes, downloading at compile time would be ideal, as this would generate a stable release, in keeping with our usual style of doing things.

the problem with remote fonts / icons presents itself especially with the tangible theme which uses both, so if somoen was using tangible they would get remote fonts without even knowing what happens to them

RE the google fonts decision (and yes this is highly controversial) https://www.cookieyes.com/documentation/features/google-fonts-and-gdpr/

[johnspackman]

As discussed on gitter, I've reverted the parts of this PR that remove the fonts from the repo, mostly for simplicity of this PR and keeping the scope of this PR (and the previous one that this is fixing) down to just making webfonts work properly and support CDNs.

In this PR, Qooxdoo provides both local fonts and a CDN url for the CSS; by default the compiler will use the local copies if available, and only use the CDN if instructed to do so. You can make the compiler prefer CDN over local fonts either with qx compile --no-local-fonts for CDN, or adding to a target in compile.json a setting where fonts.local is false

Downloading fonts would be nice, but there is no standard for how to do it or find the resources, or how to then relate the font files to @font-face definitions etc which IMHO could end up being a bit of a voyage of discovery (maybe), and pressures of work mean I'm avoiding it in this PR.

IMHO a good solution would be to build a package to contain the font and it's files, which would be hand-bundled up into a specific package version. We would also add a compiler JSDoc such as @needsPackage(some.package.name) which tells the compiler to automatically download and install the correct package to add to the build. This has the advantage that whole libraries of code can be added declaratively, and solves another outstanding issue which is dependencies between packages (which currently has to be done manually).

[oetiker]

hmmm guess I am missing the point a bit here ... are we discussing if letting the user know that a function is returning a promise is something the user should know or not ? In my eyes it is essential for the user to know, because a promise has either to be awaited or then used as a promise object ... how should Iproperly use a function if I didn't know this information?

[derrell]

That's exactly what I'm trying to get across, @oetiker. Thank you for summarizing.

[johnspackman]

are we discussing if letting the user know that a function is returning a promise is something the user should know or not ? In my eyes it is essential for the user to know, because a promise has either to be awaited or then used as a promise object ... how should Iproperly use a function if I didn't know this information?

You would know because the function is declared using the async keyword. And the API viewer would have the bug fix where it flags up that the function is async in the documentation

[johnspackman]

However, which is the correct code out of these two examples:

Example A

  myObject.doSomething();

Example B

  await myObject.doSomething();

[derrell]

However, which is the correct code out of these two examples:

Example A

  myObject.doSomething();

Example B

  await myObject.doSomething();

How would you know which of those is correct if you don't know whether the method returns a Promise? That's exactly why I'm saying that the JSDoc needs to tell you that it returns a Promise, until such time as the API viewer can otherwise tell you.

[johnspackman]

How would you know which of those is correct if you don't know whether the method returns a Promise?

Which method? Where is the documentation for that method? Javascript is a dynamic language, so unless you know the object you don't know whether it is async or not.

You can't get overly hung up on what that function is if you don't know what it is.

That's exactly why I'm saying that the JSDoc needs to tell you that it returns a Promise, until such time as the API viewer can otherwise tell you.

Then what would contribute to the project would be to accept this PR on the basis that it is following the convention of the project in the way that it is documenting async functions, and then to upgrade API viewer to fix the problem.

It would NOT be contributing to the project to modify the JSDoc instead of fixing the bug in the API viewer.

[cboulanger]

Hi, I think @derrell is correct, though. The JSDoc documentation also specifies that async methods explicitly return a Promise via the @return tag (https://jsdoc.app/tags-async.html) . That's also what I would expect. If I see a @return tag, I want to know what is actually returned, not what is implicitly returned. My 2 cents, FWIW ...

[johnspackman]

If I see a @return tag, I want to know what is actually returned, not what is implicitly returned.

But it's not returned if you're awaiting it and writing code that actually looks like async code.

The fact that the function being called is async means that you know to call it with await - although it might not be async, but that's fine.

When you use await and async the language is hiding that promises are returned at all.

eg

async function abc() {
  let value = await getAnInteger();
  console.log(value); // some number
}

Promises just are not mentioned

[johnspackman]

Anyway, this discussion is outside the scope of this PR.

[johnspackman]

@johnspackman tried to compile with your PR compiler but still get errors (initialiseFonts). Maybe I do something wrong. Project-theme which I use with webfonts: https://github.com/scro34/Bernstein . I thought mb better to really to move new functionality of webfonts to 8.0 beta and in 7 version just somehow suppress annoying warnings.

I can't reproduce this - I've checkout Bernstein and added it to my demo app with Bernstein as the theme and it compiles OK; also compiled the Bernstein project. I get warning about deprecated webfonts but that's it.

After you've checked out the latest code, did you run ./bootstrap-compiler?

[goldim]

@johnspackman I did run ./bootstrap-compiler. I will recheck it once again a little later.

[goldim]

@johnspackman the error happens at runtime not at compile time. You can repeat it with Bernstein project. Just run qx serve -S and see the error in console. I've also tried qx migrate but didn't help.

[hkollmann]

@derrell : Could you accept the documentation of return for now? I looked through the framework. This type is used often for async function. IMHO we should discuss how to handle this sort of return values and change it consistency in the whole framework. But that is another task.

[hkollmann]

@goldim : This exception also happens with qooxdoo v7.5.1 - so this is not a problem of this PR!

[goldim]

@hkollmann no, it doesn't. Just check it with master branch of qooxdoo but with this PR my workign app and theme don't work.

[hkollmann]

Interesting. When i build the bernstein demo app with current master i get the same error as with this branch

[goldim]

@hkollmann I rechecked: using qooxdoo master branch and Bernstein theme the demo app does work.

1. git clone https://github.com/scro34/Bernstein
2. npm install @qooxdoo/framework
3. npx qx serve -S --clean

[derrell]

@derrell : Could you accept the documentation of return for now? I looked through the framework. This type is used often for async function. IMHO we should discuss how to handle this sort of return values and change it consistency in the whole framework. But that is another task.

Sorry, but no. As written, it generates incorrect documentation, and I don't want to approve that. I offered to go back through all async functions and review their documentation, if/when the compiler is made to generate an indication in the JSON file that a method is async, and the API viewer is made to show methods as async. Until then, I believe this should not be used as is. And "It's broken elsewhere" isn't, in my view, an adequate excuse to push a new commit that generates broken documentation. Rather, it's a reason to go and fix the documentation on all of the other cases of broken API documentation.

If everyone else on the team thinks that generating inaccurate documentation can be ignored here, I'll accept that view. So far, though, two others on the team have indicated their agreement with me.

[johnspackman]

Sorry, but no. As written, it generates incorrect documentation

No, it does not. It is consistent with many (if not the majority) of use cases in Qooxdoo.

And "It's broken elsewhere" isn't, in my view, an adequate excuse to push a new commit that generates broken documentation. Rather, it's a reason to go and fix the documentation on all of the other cases of broken API documentation.

Actually, the proposal was to correct the Api viewer to show async functions and correct the documentation so that it is consistent.

But this PR is not the correct place to debate a policy change across the whole of Qooxdoo.

This PR meets the standard previously (currently) defined by the group.

[johnspackman]

If everyone else on the team thinks that generating inaccurate documentation can be ignored here, I'll accept that view. So far, though, two others on the team have indicated their agreement with me.

I think that @oetiker said that he wanted to know that the documentation flagged that something was async.

@cboulanger agreed with you, and proposed that we follow jsdoc (https://jsdoc.app/tags-async.html) more strictly - I would like to look at that also, because it looks like that link says we would also have to tag the function with @async, eg an async function would need to be flagged as async three times - once in code, and twice in jsdoc:

/**
 * @async
 * @return  {Promise}
 */
async doSomething() {
}

Personally I think that JSDoc is going a bit far there, especially as we only need the mandatory async keyword in code.

But again....This PR is not the place to debate policy changes, and I would greatly appreciate it if @derrell stopped trying to derail my PR (on what is IMHO a minor point) and instead prioritised getting this fix in place.

[derrell]

And just to be clear, I'm not arguing that the documentation is necessarily incorrect when viewed in the source code. It could be shown either way, there, and if the source code were our official source of API documentation, I might well acquiesce. Our official source of API documentation, however, is not the source code; it's the API viewer, and this JSDoc generates incorrect documentation in our official API documentation. Until the compiler and API viewer are modified, the only way to get accurate documentation visible in our official source of documentation, is to modify the JSDoc to show the "actual" return value (a Promise) even though one could argue that that's a behind-the-scenes operation and shouldn't be indicated in the JSDoc because the async indicator tells you so. I'll be happy to accept @johnspackman's point of view on that once our official source of API documentation reflects it. Until then, we need to take the other view on async functions: that they do, actually, return a Promise, so that our API Viewer shows correct documentation.

[johnspackman]

https://github.com/qooxdoo/qooxdoo/pull/10574

[johnspackman]

https://github.com/qooxdoo/qxl.apiviewer/commit/e546e617ea4129ebfb12d5450d3d8f95b9bc42ff

[johnspackman]

Until the compiler and API viewer are modified, the only way to get accurate documentation visible in our official source of documentation, is to modify the JSDoc to show the "actual" return

Or, as you said yesterday, the actual underlying problem could be fixed (see above, fixed in the compiler and API viewer).

Which in this case turned out to be a lot less work than hand editing all those files.

And which, given you felt so strongly about it, you could easily have done instead of blocking this PR.

[johnspackman]

@goldim I reproduced that crash in Bernstein, should be fixed now

[goldim]

@johnspackman Great! Thanks! I will check it asap.

[hkollmann]

@derrell : Could you approve now?

[derrell]

@derrell : Could you approve now?

Yes, with recent changes, the API viewer now indicates which methods are async. That resolves my objection. (We may need to rerelease the public API viewer concurrently, to ensure it has the correct JSON data that has the new information.)