Narrow numbers to const enums

[ryantrem]

Babylon has a lot of APIs where a property or variable type is just number, and you typically set the value based on a static property of a class (though I've seen plenty of code where it was just set to a number directly). For example:

scene.fogMode = Scene.FOGMODE_EXP;

Since the type of fogMode is just number, it can be hard to know what value it should be set to (especially for folks who are new to the Babylon API and not aware of this repeated pattern), and is also error prone in that you can easily assign it to an invalid numeric value and not know until runtime.

It seems like one simple fix for this could be do to define light weight const enums, something like:

// Newly introduced const enum for fog modes,
// with values matching the existing values
const enum FogMode { 
  None = 0,
  Exp = 1,
  Exp2 = 2,
  Linear = 3,
};

class Scene {
  public static readonly FOGMODE_NONE = FogMode.None;
  public static readonly FOGMODE_EXP = FogMode.Exp;
  ...

  public set fogMode(value: FogMode) {
  ...
  }
}

// New standard way of setting fogMode - immediately discoverable
scene.fogMode = FogMode.Exp; 

// Existing code using the old pattern still works
scene.fogMode = Scene.FOGMODE_EXP;

// Same existing code that was using numeric values directly
scene.fogMode = 1;

// But trying to assign an invalid value is also a compile time error
scene.fogMode = 5; // ERROR

It seems like this approach has several benefits:

1. It's backward compatible
2. It's much more discoverable
3. It's less error prone
4. It has no performance overhead (since const enums compile down to just constant values)

I see a few open questions with this approach as well though:

1. If the const enums are not preserved in the built JavaScript, then JavaScript source must be written like it is today (with "legacy" static readonly properties) and the doc comments would be correct for TypeScript but not for JavaScript.
2. If the const enums are preserved in the built JavaScript, the built JavaScript size will increase. The change in this PR showing the pattern with fog mode increases the dev build of babylon.js by 880 bytes. I'm not sure how to build the fully minified JavaScript though (@RaananW? 🙏), so maybe it would be less in that case.
3. There can be interesting cases where the change can result in a TypeScript error in existing (incorrect but functional) code. One example can be seen in this PR, where there was a condition like this: if (scene.fogMode && scene.fogMode !== 0). This code is technically incorrect, or at least redundant as it is essentially checking if fogMode is zero twice. With the proposed change, this line becomes a compile time error. If the first part of the condition is false, then fogMode is not zero, and so fogMode is narrowed to 1|2|3, which results in the second check producing an error (since 0 is not part of the 1|2|3 union). I would say technically this is actually catching a programming error in the code which is good, but it does mean existing code that does not produce TypeScript errors could produce TypeScrpit errors with this change.

I made this a draft PR to get feedback on the proposal. If folks think the pros outweigh the cons, then I would apply this pattern to many more cases. Thoughts?

[bjsplat]

Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s).
To prevent this PR from going to the changelog marked it with the "skip changelog" label.

[bjsplat]

Snapshot stored with reference name:
refs/pull/14076/merge

Test environment:
https://babylonsnapshots.z22.web.core.windows.net/refs/pull/14076/merge/index.html

To test a playground add it to the URL, for example:

https://babylonsnapshots.z22.web.core.windows.net/refs/pull/14076/merge/index.html#WGZLGJ#4600

Links to test babylon tools with this snapshot:

https://playground.babylonjs.com/?snapshot=refs/pull/14076/merge
https://sandbox.babylonjs.com/?snapshot=refs/pull/14076/merge
https://gui.babylonjs.com/?snapshot=refs/pull/14076/merge
https://nme.babylonjs.com/?snapshot=refs/pull/14076/merge

To test the snapshot in the playground with a playground ID add it after the snapshot query string:

https://playground.babylonjs.com/?snapshot=refs/pull/14076/merge#BCU1XR#0

[RaananW]

I'll start by saying that I am all for optimizations :-)

We had a long discussion about const enums a long time ago.

Some background (I noticed you addressed some of these points in your description) -

The main issue with const enums is the fact that they are typescript-only feature. So if we already have an enum, they cannot be converted to const enums, because this will not be back-compat. Performance-wise it is much better than standard enums. This is why we have been doing this hybrid public static mode, instead of using enums on code that requires high-performance.

Personally - I would rather not extend the classes with more public static variables. One reason is that it simply expands the already-large classes that we have, and the other is discoverability. For example, if the example you provided - why would the FOGMODE variables be located in Scene? And though in this examples they do have a proper naming convention, a lot of the times they don't - like the Mesh's side orientation options, for example.

But I believe your suggestion is to improve the already-existing values with const-enums to improve all of the issues I have mentioned before. This maintains back-compat (because JS already has the solution) and (as you say) improves discoverability for typescript devs.
Are there many cases we have those kind of approach? We did try to solve it with the Constants file, which acts roughly like a const enum, in a sense that the build system injects its values to the compiled sources in order to achieve a smaller footprint. The constants are still exported.

In regards to your question - I wonder if the minified version increased as well? The non-minified version's size increased probably due to the comments typescript adds when using const-enums:

export class Scene {
}
/** The fog is deactivated */
Scene.FOGMODE_NONE = 0 /* FogMode.None */;
/** The fog density is following an exponential function */
Scene.FOGMODE_EXP = 1 /* FogMode.Exp */;
/** The fog density is following an exponential function faster than FOGMODE_EXP */
Scene.FOGMODE_EXP2 = 2 /* FogMode.Exp2 */;
/** The fog density is following a linear function. */
Scene.FOGMODE_LINEAR = 3 /* FogMode.Linear */;

Minification should remove those and I expect it to eventually have the size size as before.

A more apparent issue would be documentation (and the playground in js mode) - we generate our typedoc from the declaration files. As many people (us included) are using declaration files in JS dev, they will still see the object "defined", but when trying to use it, they will find that it is not.:

A simple PG - This is expected to work, but it fails, as the object is undefined - https://playground.babylonjs.com/?snapshot=refs/pull/14076/merge#9571GM

TL;dr - I think it's a great suggestion, but it has some aspects that might be problematic. I think the better approach to solve this is use the same architecture as the constants, or maybe even find a better one - a solution that doesn't generate a class full of "public static"s, but still fully support both JS and TS

[ryantrem]

Thanks for sharing your thoughts @RaananW! To clarify one thing though, TypeScript const enums can be a TypeScript only feature, but they can also still be used in JavaScript with compiler configuration. Specifically, if you enable the TypeScript preserveConstEnum compiler flag, then the built JavaScript does have a representation of the const enum, but it is the same as a regular enum (with the back mapping and everything, which makes it semantically different from the TypeScript const enum). I'm not sure why the compiler does it this way... I actually tested this all out a couple months ago and observed the const enum compile down to just:

var FogMode = {
  None: 0,
  Exp: 1,
  Exp2: 2,
  Linear: 3,
};

But now I can't seem to repro this, so I don't know if I was using the TS Playground with a pre-release TS version, or maybe I saw it with Babel or something, I can't remember 🤷‍♂️. I think if we could get the TS compiler to emit the above, it would be pretty good. But the full enum generated with preserveConstEnum is bigger and semantically different, so it's not great.

Also, what I saw locally is that the full enum existed in the dev build of the babylon.js file, though as you mentioned it seems to not exist for the Playground build associated with my PR. I don't know why the behavior is different, and I don't know why in my local dev build the enums are emitted to JS since I can't find the preserveConstEnum compiler flag being specified anywhere in the BJS repo (and the default value of the flag is false). How can I build the fully minified JS locally (the same thing that would be pushed to npm)?

[RaananW]

A nice thing that preserveConstEnum does (which TBH I wasn't aware of) is that it assigns the value while keeping the enum. So, apart from the generated enum (which is string-based) the values are embedded wherever they are used in the framework. I think it might be something we can consider using.
If TypeScript will ever deal with this issue - microsoft/TypeScript#37282 it will be the best - avoid the enum reverse mapping when using const enums.

We could add some form of a transformer to generate objects instead of enums when using const enum. But that will complicate the build process a bit more. Need to think how possible it is and how long it will take to implement.

All in all, i love the suggestion. I think we could benefit from const enums, and I think that in certain cases we could use them instead of Constants or public statics. Especially if we enable the preserveConstEnum flag.

To your last question:

How can I build the fully minified JS locally (the same thing that would be pushed to npm)?

npm run build:babylonjs will run a UMD build of core. You can also run npx nx run babylonjs:build which will practically do the same, but by changing the babylonjs to any other package you can build them as well (for example npx nx run babylonjs-gui:build)

[github-actions]

This pull request has been marked as stale because it has been inactive for more than 14 days.

[github-actions]

This pull request has been marked as stale because it has been inactive for more than 14 days. Please update to "unstale".

[deltakosh]

Closing with no activity