[Bug] Computed property smashing regression on 3.21.3

[ef4]

🐞 Describe the Bug

In an app using ember-cli-typescript, a controller can declare the type of its model like this:

export default class Application extends Controller {
  model: Whatever;
}

This works fine in 3.21.2. But using ember-source 3.21.3, it causes this.model to be undefined.

🔬 Minimal Reproduction

Run this app and see it crash: https://github.com/ef4/--bug-repro

Switch the app to use ember-source 3.21.2 and see it not crash.

Or comment out the type declaration for model in app/controllers/application.ts and see it not crash.

😕 Actual Behavior

The model computed property appears to be getting smashed by typescript's compiler output (which emits something like Object.defineProperty(this, 'model', void 0) in the constructor).

🤔 Expected Behavior

Should behave the same as in ember 3.21.2.

[pzuraq]

@ef4 I'm not sure what changed here exactly, but I believe TypeScript users should be using declare model: Whatever; at this point, because in the future that will always clobber, and there's not really a way around that (except redefining the property after the class has been instantiated)

[ef4]

Yeah, I agree the usage isn’t great. I was just surprised to see the behavior change in a patch release.

[pzuraq]

So looking at the code for model in particular, it hasn't changed in about a year: https://github.com/emberjs/ember.js/blame/master/packages/%40ember/controller/lib/controller_mixin.js#L48-L56

I don't believe we modified anything about this in particular recently. Is the Ember patch version the only thing that changed?

[ef4]

Bisected to f40af11, which is #19172

[ef4]

I don't think this is specific to model, I think it probably affects all computeds.

[rwjblue]

@ef4 - I think #19178 is what you are reporting, can you confirm?

[pzuraq]

@ef4 oh interesting, I think this may have actually been another more subtle bug that actually was fixed by that PR. What would have been happening before is:

1. We define the class with a getter/setter on the prototype
2. TS class shadows the getter/setter with defineProp on the instance
3. Ember.set sets the value on the instance because it thinks it's a plain value. It's set correctly, but it's no longer a tracked property.

Now, it's being set on the descriptor, but the getter/setter that access the descriptor are still shadowed. I wonder if we can assert if the accessors are shadowed like this?

[ef4]

@rwjblue I don't think so. I just checked and PR #19178 does not fix this bug.

[rwjblue]

Thank you for checking @ef4.

@pzuraq - Think you may have time to dig in? I'm trying to gauge releasing 3.21.1 as 3.21.4 while we work through this (and we should consider reverting the code changes)...

[rwjblue]

Also, we should try to get a failing test case PR up based on @ef4's demo above so we can confirm any potential fixes work properly.

[pzuraq]

@ef4 based on the description I gave above, do you agree that the previous behavior was actually buggy, and even though this "broke" more visibly we probably cannot actually fix it?

[ef4]

Can we confirm your theory with a test? If it's true that there was missed autotracking before, that's a pretty convincing reason to keep things the way they are now.

[ef4]

I would guess that many apps haven't tried 3.21.3 yet, given the prevalence of lockfiles and the six-week cycle. So I don't have a sense for how common this bug will be in the wild.

I'm going to try to loop in some of our typescript experts to answer the following question: Ping @chriskrycho, @dfreeman, @mike-north, @jamescdavis, @chancancode!

Do people use this pattern a lot? Specifically, declaring a type for a computed property that comes from your base class like:

export default class Application extends Controller {
  model: Whatever;
}

(not necessarily just for Controller#model, but any baseclass-provided computed.)

It breaks in ember 3.21.3. Do you have typescript codebases where you can look for this problem?

[pzuraq]

@ef4 right, but the real issue is that even if it appears to work at first, the next time the model hook is fired it will not properly update, since model has been overwritten. Here's a twiddle demonstrating what I mean: https://ember-twiddle.com/6b69aa5f4e56e10061ca0061df788056

I don't think we can fix this bug in a way that would not cause the bug in the twiddle to resurface.

Edit: Ah, I only saw your most recent comment, not the one right before it. I think the twiddle demonstrates the missing autotracking behavior, I think we definitely should add this as a test.

[chriskrycho]

I expect it’s going to be extremely common in existing TypeScript codebases. While the declare syntax has existed for the better part of a year, we have still seen a lot of questions come up in Discord about the ways that the existing syntax breaks (and has for a while) in certain circumstances.

We have also been telling people for the better part of a year that the pre-declare approach is broken in many circumstances, and TS is not technically supported as of yet, so this is the kind of thing TS users generally understand is a risk. Obviously we’d prefer to avoid breakage where possible, though.

(I’m also going to include this in the list of kinds of specific things we need to mitigate as I’m working on the design for publishing types officially.)

[pzuraq]

I think what we should do here is have set detect if a @tracked or @computed value has been shadowed, and throw an error in DEBUG if so. With the current behavior, that is definitely a bug, since the value is set via meta in a sidechanneled way and appears to be swallowed. Even if we restored the previous behavior, I think in most cases shadowing was likely a latent bug that would bite people in much less obvious ways. What do y'all think?

[chancancode]

Do people use this pattern a lot? Specifically, declaring a type for a computed property that comes from your base class like

I think we would probably have used declare here. The reason we didn't is either because the code was written before we were able to use declare, or in this case, it was because the code is in a JavaScript file which the declare syntax is not available.

I guess I would be more worried about the JS case since it seems quite common to use the field syntax for documentation purposes.

Anyway I agree as typescript users this seems like the kind of thing that we fix pretty regularly so it's not a super big deal as long as it's easy to figure out where/why things broke.

[pzuraq]

I suppose one valid case where shadowing may make sense is if you wanted to add a getter/setter to overwrite a computed in a subclass, for instance. This could also technically work for tracked properties, but it feels much sketchier to do that.

If shadowing is valid in some cases, is there a way to warn users when they are doing in unintentionally, e.g. for documentation purposes?

[boris-petrov]

I'm not sure I correctly follow that issue but we're on the latest Ember (I tend to upgrade all the time) and we have a bunch of public model!: Model; in controllers and that seems to work fine... in any case, should we be using this or public declare model: Model instead?

[chriskrycho]

You should! That’s the correct way to define a type with these class field semantics (that is, defined by an external-to-the-class caller). We used and recommended the other approach because it’s all we had, but the declare syntax was designed to handle exactly the Ember scenario (ours was not the only use case but was definitely part of the input to the design process for TS).

[pzuraq]

We discussed this, and the solution we're planning to implement is to assert when this occurs in general to let users know that something may be being shadowed when they didn't realize it. It seems like this is pretty rare in general (especially given the only failure reported so far was due to type definitions, and not legitimate shadowing via inheritance) and it's also problematic as we were not applying it consistently previously - CoreObject.create() would have had this same issue prior to this change.

We do also plan to restore the previous behavior on release however, to minimize disruption for now and also stabilize the underlying fixes so they can be backported to LTS. We don't want this change to cause issues in a patch release there.

[rwjblue]

I'd like to understand what is actually causing the Object.defineProperty in the child class?

I tried this snippet:

class Parent {
  @tracked foo = "whatever";
}

class Child extends Parent {
  foo: string;
}

And in both Babel Playground and Typescript Playground result in:

class Child extends Parent {
}

As far as I can tell that will not cause the bug reported here.

---

@ef4 - What transpilation were you using? What version of ember-cli-babel, @babel/preset-typescript, typescript, etc?

[boris-petrov]

@rwjblue - on the TypeScript playground that you gave, click TS Config and in Advanced check useDefineForClassFields - that will lead to the desired result.

[ef4]

@rwjblue I shared a complete minimalist reproduction above. The line that causes trouble is here:

https://github.com/ef4/--bug-repro/blob/7d569f52a00636d7c5506849d5ca9eb26107f81c/app/controllers/application.ts#L8

The app is using ember-cli-babel 7.22.1 and ember-cli-typescript 4.0.0, the full yarn.lock is here.

[rwjblue]

This was fixed in #19197 and included in 3.22.1.