Minification can break queries due to Component name collision

[zeddic]

Summary

Running an application through a minifier w/ ES5 compatibility can break system queries. This can be caused by how the minifiers represent classes, which can include giving the constructor functions a generic name (eg "n") which may collide.

What causes the error?

I've traced the error down to the Utils.queryKey function which generates a unique key given a list of Component references. It uses Component.name to generate the key, which uses the function's name. Given the function names could be the same, this will result in query key collisions.

Proposed solution

I saw that a recent pull request started stamping unique IDs onto component references using the _typeId property. I think this could be addressed by having queryKey use this ID rather than the function name.

Details on how the collisions can occur with minification

In my case I saw the error when using Terser and outputting ES5 code. The configuration option keep_fnames, which defaults to false, was renaming the constructor fn to all the same name for all the classes. For example, in psuedocode, given something like:

class Test1{}
class Test2{}

it would generate something (roughly) like:

const Test1 = (function() {
  function n() {} // <-- constructor fn
})();
const Test2 = (function() {
  function n() {} // <-- constructor fn
})();

which would cause both classes to have the name n.

[zeddic]

After more consideration, using the _typeId as it currently is comes with a consequence. Since _typeId is only generated upon Component registration, this would require that all Components be registered before Systems, which isn't great.

A couple of other ideas:

1. The getTypeId(Component) logic could be refactored out of ComponentManager and into a utility function. It could inspect the component for an existing _typeId, and return it if set, otherwise it would generate a new _typeId and stamp the component with it on demand. This would remove the requirement that Components must be registered first.

2. Use the new Component.displayName property via Component.getName(). This does require that people using an obfuscator set a displayName for all their components though, which may not be intuitive.

Thoughts? I lean towards option 1.

[zeddic]

Here is a potential fix using option 1. https://github.com/MozillaReality/ecsy/compare/dev...zeddic:fix-query-collision?expand=1

If this looks OK I'll send it as a pull request.

[fernandojsg]

Hi @zeddic thanks for the clear description on the issue. I had the update on the queryKey in the TO-DO list with the recent PR to remove any .name usage around the code and move to the typeId but I missed it in the last PR :) I've been looking at your PR and it looks cool, please feel free to open the PR and I'll get that merged. Thanks!

[fernandojsg]

Looking at the code the main concern I have is with using getTypeId() everywhere as replacement for _typeId:

Component.getTypeId = function() {
  if (this._typeId !== undefined) {
    return this._typeId;
  }

  this._typeId = nextComponentId++;
  return this._typeId;
}

Mainly because of the overhead this could introduce (versus accessing just the variable), I'd like to measure it. Another option is, instead of replacing all the _typeId references with getTypeId, keep the ones accessing the variable in the cases where we know that the component should have already the entityId (eg: addComponent or removeComponent from an entity as it should be already registered).

I'll cherrypick the queryKey changes and I'll keep investigating how to proceed on the getTypeId, feedback welcome! ;)

[fernandojsg]

Resolved this in #222

[fernandojsg]

Fixed by #222