Types: Convert ResourceLoaders to ES6 classes

[donmccurdy]

Description

Converts ResourceLoader and its subclasses to ES6 classes.

1. Grepped for new \w+Loader\.[a-z] invalid use of new on non-constructor factory methods (none found)
2. Grepped to find subclasses of ResourceLoader, and converted them. DracoLoader isn't a subclass, but is included anyway.
3. Using the script from Types: Bulk conversion to ES6 classes #13125, run node ./lebab-batch.js "packages/engine/Source/Scene/*Loader.js" and node ./lebab-batch.js "packages/engine/Source/Scene/Model/*Loader.js". Manually handled tool warnings in GeoJsonLoader.js.

In this case I think it's simplest to get the conversion to ES6 classes done first, and enable type checking (with many more manual fixes) later. As before, review is much easier with GitHub's split view, and whitespace changes hidden (?diff=split&w=1). Lebab shouldn't change the order of class members unnecessarily.

Issue number and link

• Use ES6 "class" as module syntax #8359

Testing plan

Mainly, ESLint and unit tests should pass. It wasn't obvious (to me) which sandcastle examples use which loaders. One way of checking that during a manual review of sandcastles is to temporarily add a small constructor in ResourceLoader:

constructor() {
  console.count(this.constructor.name);
}

ESLint verifies that all subclasses invoke super() in their own constructors.

Author checklist

• I have submitted a Contributor License Agreement
• I have added my name to CONTRIBUTORS.md
• I have updated CHANGES.md with a short summary of my change
• I have added or updated unit tests to ensure consistent code coverage
• I have updated the inline documentation, and included code examples where relevant
• I have performed a self-review of my code

[github-actions]

Thank you for the pull request, @donmccurdy!

✅ We can confirm we have a CLA on file for you.

[javagl]

• Checked out the branch
• npm install ✔️
• npm run eslint ✔️
• npm run prettier-check ✔️
• npm run test ✔️ (except for unrelated things)

I won't claim to have read everything in the GitHub diff-view, but spent probably a bit more time with comparing both sides than would have been necessary. And I noticed something that I hadn't seen before:

(At the end of the diff). Some sort of easter egg for large changes, apparently. So that's something.

In addition to DracoLoader, the change covered two classes that are not in the ResourceLoader hierarchy, but only used by resource loaders, namely CreateIndexBufferJob and CreateTextureJob, so that's OK. I also looked at some classes locally, and nothing stood out.

Somewhat an aside: I sometimes wondered what these blocks

if (defined(Object.create)) {
  B3dmLoader.prototype = Object.create(ResourceLoader.prototype);
  B3dmLoader.prototype.constructor = B3dmLoader;
}

have been doing, but fortunately, they are gone now. (I probably don't really want to know ... ~"one of these JavaScript things")

What I'd consider to be a remaining TODO, but after this PR (and not in the scope of this PR) is what I already mentioned in #13126 (comment) : The loaders heavily use the file-scoped functions antipattern. (I'll just call it an antipattern from now on - everybody can feel free to challenge me here). I'm not sure where and how this is supposed to be tracked. (It may be worthwhile to do this before larger changes, like the one suggested in #13052 , but it's not clear where, when, and how that could start either).

[donmccurdy]

Thanks @javagl! About the @private tags and file-scoped functions, I've filed a new issue here:

• Types: Separating 'private' vs. 'internal' visibility #13205

It's somewhat more focused on the interaction of @private tags with type checking, so feel free to open a separate issue specifically about file-scoped functions if you'd prefer.

[jjspace]

I sometimes wondered what these blocks have been doing

@javagl just in case you're interested this is the "right way" to extend objects in a pre keyword class JS world. I talked about this more in a previous TS comment: #10455 (comment)

The loaders heavily use the file-scoped functions antipattern. (I'll just call it an antipattern from now on - everybody can feel free to challenge me here).

I think it's also worth having a discussion around this too. I don't think there's anything wrong with "file scoped functions". In fact I'd actually personally encourage the use of them where it makes sense. Of course the "where it makes sense" may require some discussion.

[javagl]

this is the "right way" to extend objects

I probably skimmed over that back then, and maybe even read it (and the linked SO Q/A). But this is the kind of knowledge that tends to fall thhough the cracks (there are just too many obscure quirks in that language).

I think it's also worth having a discussion around this too. I don't think there's anything wrong with "file scoped functions". In fact I'd actually personally encourage the use of them where it makes sense. Of course the "where it makes sense" may require some discussion.

The Coding Guide suggests them for "better encapuslation" (which is kinda funny and ironic when looking at some._access._chains._through._private._properties that they sometimes contain). And specifically, in a way where they replace actual instance functions. Of course, there's nothing wrong with a file-scoped function per se (except for my Java heart bleeding a bit), but these should be pure functions (and perferably, in files that match their names). For private instance functions, the _ and @private/@internal should be enough.

[donmccurdy]

Just realized I forgot to split the constructor JSDoc comments out into class-level and constructor-level comments:

Before

/**
 * Description of my class.
 * @param {number} a
 * @param {number} b
 * @constructor
 */
function MyClass (a, b) {}

After

/**
 * Description of my class.
 */
class MyClass {
  /**
   * @param {number} a
   * @param {number} b
   */
  constructor(a, b) {
  }
}

I'll do that in a followup PR and add it to the checklist for future PRs, and maybe add a warning in the lebab-batch.js script to check for @constructor tags as a reminder.

EDIT: Followup PR:

• Types: Fix JSDoc on ResourceLoader constructors #13209