[IE11] @tracked property initializer is not applied

[buschtoens]

In IE11 the property initializers for the @tracked decorator are not applied. The following test succeeds in Chrome, but fails in IE11.

tests/unit/tracked-test.js

import { module, test } from 'qunit';
import { setupTest } from 'ember-qunit';
import { tracked } from '@glimmer/tracking';

module('Unit | @tracked', function(hooks) {
  setupTest(hooks);

  test('it applys the initializer', function(assert) {
    class Subject {
      @tracked
      property = 'foo';
    }

    const subject = new Subject();

    assert.strictEqual(subject.property, 'foo');
  });
});

Reproduction repo here: buschtoens/repro-tracked-ie-error

[buschtoens]

I added another test case, showing that a re-render is properly triggered nevertheless, when setting the property:

tests/integration/tracked-test.js

import { module, test } from 'qunit';
import { setupRenderingTest } from 'ember-qunit';
import { render, settled } from '@ember/test-helpers';
import hbs from 'htmlbars-inline-precompile';
import { tracked } from '@glimmer/tracking';

module('Integration | @tracked', function(hooks) {
  setupRenderingTest(hooks);

  test('it re-renders, when the value is mutated', async function(assert) {
    class Subject {
      @tracked
      initializedProp = 'foo';

      @tracked
      uninitializedProp;
    }

    this.subject = new Subject();

    await render(hbs`
      initializedProp = {{this.subject.initializedProp}};
      uninitializedProp = {{this.subject.uninitializedProp}};
    `);

    // Only this first assertion fails in IE11, the rest passes.
    assert.ok(this.element.textContent.includes('initializedProp = foo;'));
    assert.ok(this.element.textContent.includes('uninitializedProp = ;'));

    this.subject.initializedProp = 'bar';
    await settled();

    assert.ok(this.element.textContent.includes('initializedProp = bar;'));
    assert.ok(this.element.textContent.includes('uninitializedProp = ;'));

    this.subject.uninitializedProp = 'qux';
    await settled();

    assert.ok(this.element.textContent.includes('initializedProp = bar;'));
    assert.ok(this.element.textContent.includes('uninitializedProp = qux;'));
  });
});

[buschtoens]

ember.js/packages/@ember/-internals/metal/lib/tracked.ts

Lines 203 to 222 in be4f59a

	get(): any {
	let propertyTag = tagForProperty(this, key);
	if (CURRENT_TRACKER) CURRENT_TRACKER.add(propertyTag);
	// If the field has never been initialized, we should initialize it
	if (!(secretKey in this)) {
	this[secretKey] = typeof initializer === 'function' ? initializer.call(this) : undefined;
	}
	let value = this[secretKey];
	// Add the tag of the returned value if it is an array, since arrays
	// should always cause updates if they are consumed and then changed
	if (Array.isArray(value) || isEmberArray(value)) {
	update(propertyTag, tagForProperty(value, '[]'));
	}
	return this[secretKey];
	},

When first accessing a @tracked property with an initializer:

• Chrome: secretKey in this === false
• IE11: secretKey in this === true

[buschtoens]

Something is really screwed up in IE11. Calling symbol(key) immediately puts the created Symbol on _target as undefined, which is why secretKey in this === true. The Object itself returned from symbol(key) also has the secretKey as an undefined property.

Somehow they all must be sharing the same base object that symbol for some reason puts a property on. I enabled includePolyfill and I have the suspicion, that the polyfill is the cause.

[buschtoens]

Indeed! For whatever reason, this line is included in the polyfill and sets the symbol on Object.prototype. WTF.

https://github.com/zloirock/core-js/blob/6a3fe85136aaae0e3b099c96a05a5ceb1f515a50/modules/es6.symbol.js#L135-L148

// L143
if (DESCRIPTORS && setter) setSymbolDesc(ObjectProto, tag, { configurable: true, set: $set });

https://github.com/zloirock/core-js/blob/6a3fe85136aaae0e3b099c96a05a5ceb1f515a50/modules/es6.symbol.js#L49-L59

// fallback for old Android, https://code.google.com/p/v8/issues/detail?id=687
var setSymbolDesc = DESCRIPTORS && $fails(function () {
  return _create(dP({}, 'a', {
    get: function () { return dP(this, 'a', { value: 7 }).a; }
  })).a != 7;
}) ? function (it, key, D) {
  var protoDesc = gOPD(ObjectProto, key);
  if (protoDesc) delete ObjectProto[key];
  dP(it, key, D);
  if (protoDesc && it !== ObjectProto) dP(ObjectProto, key, protoDesc);
} : dP;

setSymbolDesc === dP in IE11. dP in turn is function defineProperty() { [native code] }, so should be dP === Object.defineProperty, but for some reason it is not.

I don't know why that is (probably just an IE quirk), but I expect setSymbolDesc to behave just like Object.defineProperty nevertheless.

[buschtoens]

IMO this is a bug upstream, but I don't have high hopes, that it will (or even can) be fixed (in a timely manner). Should we maybe workaround it instead?

[rwjblue]

We can change the implementation to avoid relying on setting arbitrary properties on the object.

Something like:

function descriptorForField([_target, key, desc]: [
  object,
  string,
  DecoratorPropertyDescriptor
]): DecoratorPropertyDescriptor {
  assert(
    `You attempted to use @tracked on ${key}, but that element is not a class field. @tracked is only usable on class fields. Native getters and setters will autotrack add any tracked fields they encounter, so there is no need mark getters and setters with @tracked.`,
    !desc || (!desc.value && !desc.get && !desc.set)
  );

  let initializer = desc ? desc.initializer : undefined;
  let secretKey = symbol(key);
  let VALUES: new WeakSet();

  return {
    enumerable: true,
    configurable: true,

    get(): any {
      let propertyTag = tagForProperty(this, key);

      if (CURRENT_TRACKER) CURRENT_TRACKER.add(propertyTag);

	  let value = undefined;

      // If the field has never been initialized, we should initialize it
      if (!VALUES.has(this)) {
        if (typeof initializer === 'function') {
          value = initializer.call(this);
        }

        VALUES.set(this, value);
      }

      // Add the tag of the returned value if it is an array, since arrays
      // should always cause updates if they are consumed and then changed
      if (Array.isArray(value) || isEmberArray(value)) {
        update(propertyTag, tagForProperty(value, '[]'));
      }

      return value;
    },

    set(newValue: any): void {
      markObjectAsDirty(this, key);

      VALUES.set(this, newValue);

      if (propertyDidChange !== null) {
        propertyDidChange();
      }
    },
  };
}

[pzuraq]

We decided on the symbol based approach for performance reasons, since accessing a WeakMap would likely be pretty slow compared to a direct slot on the object. If we want to avoid arbitrary properties on the object in development mode for better DX I think that would be reasonable, but I do think we should continue with the symbol based approach for prod.

We also were planning on creating a Babel transform specifically for @tracked so that initializers would still run even with the decoration, so the symbols would get slotted on construction and not lazily (which would prevent the shape from changing unpredictably). This is what the future spec is actively being aimed at, since it'd be the most performant option.

[rwjblue]

I'd be pretty surprised if the WeakMap solution has different performance characteristics than the this[secretKey] one. Seems like we could test though.

Either way, we have to support IE11 and the core-js symbol stuff that is linked above is pretty odd (all generated symbols are defined on Object.prototype!?), I think we probably have to go with the WeakMap solution...

[pzuraq]

Maybe @krisselden can chime in here, this was based on our conversations with him and his experience with WeakMaps vs direct slots. We did switch to the VALUES based approach once, but based on his advice we switched back to symbols and slotting.

I actually think the solution of adding the babel transform would fix the problem here, since we don't actually want to assign values from the initializer lazily, even if we go with the VALUES approach.

[buschtoens]

Assuming that we want to stick with the usage of symbols and want to go with the Babel transform, what you'd envision is a transform that basically does this...?

class Foo {
  @tracked
  bar = 'qux';
}

becomes

class Foo {
  @tracked
  bar;

  constructor() {
    this.bar = 'qux';
  }
}

[pzuraq]

Yup, exactly! I think it would have to work on the class itself so it could get a lock on the whole class body, and it would need to be one of the first transforms to run (before class fields and decorators), but it should be a pretty simple transform to write.

[buschtoens]

Coincidentally I had to do something very similar in machty/ember-concurrency-decorators#50. I'll try to spend some time on a PoC later.

[rwjblue]

@pzuraq - Why do we have to eagerly initialize the values when using the WeakMap solution I suggested above? I understand that it is more spec compliant, but thats ultimately a babel issue (with the current plugin) not something we should have to work around in a special way.

Specifically, if we are using a WeakMap we don't have any shaping issues at all. The only shaping issues we currently might have is because we are adding more fields to the object.

[buschtoens]

Made a Babel transform here: babel-plugin-force-eager-initialization

It's probably far from perfect, but IE11 works now! 🎉

[pzuraq]

@rwjblue I was mainly thinking to avoid the in check every time we look up a property, and also so the shapes of the objects in the map would be consistent (unsure if that would matter much though, since the values would only be accessed locally? Not much of a chance for inline caching, etc.)

@buschtoens this looks great! I think this would be perfect, assuming we want to go this direction 😄

[buschtoens]

FWIW I think the symbol descriptor gets set on Object.prototype, so that if you set the symbol on any other object, it is non-enumerable. AFAICT there wouldn't be any other way to do this.

[rwjblue]

@pzuraq right, my point is that with a WeakMap we don't have shaping issues (we don't add any additional fields to the object at all), and we don't need an in check (instead we'd have a .has on the WeakMap). I do agree that the TS style for initialization is better (moving it to the constructor), but I just don't think we can rely on it even if we use the babel plugin that @buschtoens made in ember-cli-babel.

I really think the idiomatic / correct path forward here is the WeakMap solution I linked to above.

[buschtoens]

From an architectural POV I would also prefer the WeakMap solution, since it is clean and requires no hackery.

IIRC the motivation to use symbols in the first place was that they apparently perform better than a WeakMap. ^{Citation needed.}

[ef4]

@buschtoens thanks for your work on this, I just hit this same bug and was able to use your babel plugin as a quick workaround.

It didn't run out of the box, I ended up copying the whole file out of the unreleased version of @babel/helper-create-class-features-plugin.

[buschtoens]

@ef4

It didn't run out of the box, I ended up copying the whole file out of the unreleased version of @babel/helper-create-class-features-plugin.

You likely have some derived classes, which run into this branch:

    if (isDerived) {
      const bareSupers = [];
      constructor.traverse(findBareSupers, bareSupers);
      for (const bareSuper of bareSupers) {
        bareSuper.insertAfter(nodes);
      }

I didn't copy over the findBareSupers.

[buschtoens]

@rwjblue @pzuraq In any case, if in the end you want to stick with symbols, I am happy to add tests around this transform, upstream it somewhere official and yield control / ownership to the ember-cli org.

If you want to go with the WeakMap, it still was a fun and rewarding learning experience for me! ☺️

[rwjblue]

FYI - I chatted a bit more with @pzuraq and @krisselden, and I think we are all on the same page now. We are going to swap to the WeakMap solution for now, and we can reevaluate later if that crops up on future performance profiles.

[rwjblue]

Submitted #18091 to migrate to the WeakMap path.