Create codemod to update legacy object usage to ES Classes

[buschtoens]

(I'm using the latest beta here.)

If ember-metal-es5-getters is enabled in a canary build and decorators are used on "old" Ember object model classes, two things related to injected properties break down.

I've made a demo here: https://github.com/buschtoens/es5-getters-bug/blob/master/app/components/my-component.js

Ember.inject.* macro

import Component from '@ember/component';
import { inject as service } from '@ember/service';
import { reads } from '@ember-decorators/object/computed';

export default Component.extend({
  store: service(),

  @reads('store') storeAlias: null
});

Assertion Failed: InjectedProperties should be defined with the inject computed property macros.

Thrown here: https://github.com/emberjs/ember.js/blob/99c84c06512d794224b2fb3f3af61d7c4f8fa8d2/packages/ember-metal/lib/injected_property.js#L48

function injectedPropertyGet(keyName) {
  let desc = descriptorFor(this, keyName);
  let owner = getOwner(this) || this.container; // fallback to `container` for backwards compat

  assert(`InjectedProperties should be defined with the inject computed property macros.`, desc && desc.type);
  assert(`Attempting to lookup an injected property on an object without a container, ensure that the object was instantiated via a container.`, owner);

  let specifier = `${desc.type}:${desc.name || keyName}`;
  return owner.lookup(specifier, {source: desc.source, namespace: desc.namespace});
}

This error already occurs in mergeMixins, because the property is directly accessed as props[key] here:

for (key in props) {
  if (!props.hasOwnProperty(key)) {
    continue;
  }
  keys.push(key);
  addNormalizedProperty(base, key, props[key], meta$$1, descs, values, concats, mergings);
}

@service / @controller decorators

import Component from '@ember/component';
import { service } from '@ember-decorators/service';
import { reads } from '@ember-decorators/object/computed';

export default Component.extend({
  @service store: null
});

Attempting to lookup an injected property on an object without a container, ensure that the object was instantiated via a container.

Pretty much exactly the same problem as described above. The only difference here is that descriptorFor(this, keyName) actually does return the correct descriptor. However, getOwner(this) || this.container is undefined since this refers to the object literal that is being passed to Component.extend(...) and not the component instance.

This error can actually be avoided with some trickery in the decorator:

export const service = computedDecoratorWithParams((target, key, desc, params) => {
  const injectedProperty = params.length > 0 ? injectService(...params) : injectService(key);
  const originalGetter = injectedProperty._getter;
  injectedProperty._getter = function(/* keyName */) {
    return this === target ? injectedProperty : originalGetter.apply(this, arguments);
  }
  return injectedProperty;
});

If this would be accepted as a fix, I'd open PRs at @ember-decorators/utils for a new util and integrate it with /service and /controller.

But of course, this unfortunately does not fix the first error.

[buschtoens]

This also appears to cause trouble without ember-metal-es5-getters in a regular Ember build. 🙁

[pzuraq]

We no longer support the usage of decorators with standard objects, since that was deprecated from decorators themselves quite some time ago. If you want to keep using decorators, it needs to be with classes. I don’t think we can reconcile these worlds without some major, major backflips.

[pzuraq]

To extrapolate a bit more on that: we fixed a major bug in decorators with the move to v2 for classes, which was calling Ember.defineProperty at the end of decorators which return a CP. This is a crucial setup step which was not occuring at all, because normally it occurs during prototype collapsing (proto in CoreObject) which does not happen in native classes. This was causing Ember Data decorators, for instance, to not function at all, and made ES5 getters break as well.

By calling Ember.defineProperty at definition time however, we are giving a fundamentally different object to CoreObject in the standard object case. It will attempt to collapse that objects’ prototype, which has already been collapsed, and that will generally cause bugs.

The best we could do is detect if the object we are decorating is a POJO or a class somehow, but I’m dubious of supporting to completely different object models for reasons like this. There are going to be many edge cases, and I believe the differences in behavior will cause very different code paths that will make maintenance and moving forward difficult.

[buschtoens]

Okay. That makes sense and I understand the reasoning. Thanks for the explanation.

So in order to upgrade, users are required to transform all classes that use decorators to the new ES6 syntax. That is gonna be a lot of work in legacy projects. 😱

I'm probably better off venturing into codemods than doing this by hand. 🙈

Feel free to close this issue.

[pzuraq]

I’m open to releasing a versions of v1.0 that contains any last fixes for ES5 getters, but yeah that’s the unfortunate thing about rolling forward, more and more projects are using ES classes and they’ll need v2 😕 sorry about this, I know it’ll hit early adopters hard.

I’m on vacation (🇳🇪) but I’m also definitely down to help with a codemod. It would be awesome to have one that automates updating to classes, and if worse comes to worst I think you could automate stripping decorators pretty easily.

[buschtoens]

No need to excuse. This is tough for early adopters, yeah, but it's a price we have to pay.

Please do enjoy your well-earned vacation and do not feel obligated to help me / us out here. I will try to get myself acquainted with codemods in the meantime. I always wanted to anyways and this is the perfect excuse for me to do so. 💪

[dfreeman]

Building a codemod for migrating to ES classes has been on my 'projects to maybe pursue' list for a while now. I don't think I'll have the capacity to drive it any time soon, but @buschtoens I'm happy to help out if that's something you want to take point on 🙂

[buschtoens]

@dfreeman Awesome! Thanks a bunch. I'm currently downgrading our app's master back to Ember 2.18 and implementing some other features underway. I hope to start working on the codemod on Monday. I'll ping you then. :)

[rwjblue]

Has anyone started down the codemod path already?

[buschtoens]

Unfortunately no. I don't work for my old employer any more and my new company is already on TypeScript. 🎉

[scalvert]

We're looking into spinning the effort around a codemod to port ember object -> es6 classes. We'll likely be resourcing it internally full-time, but would love community help on it. I've spun up a repo to help define the specifics of the migration path. Feel free to add details there.

[Techn1x]

I've just updated to Ember 3.1 and now my app is broken, with the error message described above
Assertion Failed: InjectedProperties should be defined with the inject computed property macros.

I've read through several github threads, and all the googling I've done for the last few hours has not helped me figure out how to fix this - is there a workaround? Do I need to update Ember to 3.2? Is there some other package I need to update?

From what I've been able to work out it seems it's because one of my packages depends on ember-popper, which depends on ember-decorators. I'm using the latest version of both (0.9.1 and 2.3.1 respectively)

The error seems to also reference AliasedProperty.get so could it be something to do with my aliases directly on the service or is this a red herring?

All of my services are injected like this;

import Component from '@ember/component';
import { inject as service } from '@ember/service';
import { alias } from '@ember/object/computed';

export default Component.extend({
  infoService: service(),
  someval: alias('infoService.somevalue')
});

Any help appreciated

[pzuraq]

I think this could appear because of one of two issues:

1. You’re somehow including an older version of Ember Decorators in your final build. This could be because of an okd transitive dependency somewhere that’s locked, if you use Yarn I would check the lockfile to see if there are any different versions.

2. An addon somewhere is using the old style of class syntax without decorators, assigning computed proerties directly to class instances:

class FooComponent extends Component {
  service = service();
}

If you’re using the old style of class syntax like in your snippet, you shouldn’t have to update anything yourself. If you could post a reproduction it would really help for debugging this.

[Techn1x]

1. I'm using npm but I checked the package-lock.json file and all references seem to be 2.3.1. Also this
   

2. I don't quite understand what you mean by old syntax. You gave a code snippet of old syntax as an example of what to look out for, then said that I'm using old syntax and that should be fine? And the two snippets are completely different?

I'll see what I can do about a reproduction

[buschtoens]

Old and incorrect

Assigning computed proerties directly to class instances:

import { inject as service } from '@ember/service';

class FooComponent extends Component {
  foo = service();
}

New and correct

Using the decorators:

import { service } from '@ember-decorators/service';

class FooComponent extends Component {
  @service foo;
}

[Techn1x]

I seem to have gotten past the error, I think it was to do with injecting services into my controller via setupcontroller in the route?

Before

import Route from '@ember/routing/route';
import AuthenticatedRouteMixin from 'ember-simple-auth/mixins/authenticated-route-mixin';
import { inject as controllerInject } from '@ember/controller';
import { inject as service } from '@ember/service';

export default Route.extend(AuthenticatedRouteMixin, {
  setupController: function(controller, model) {
    this._super(controller, model);

    controller.setProperties({
      screenSize: service(),
      infobarService: service(),

      cachedAsset: undefined,
      currentAsset: alias('infobarService.asset'),

      // Determine if infobar content should show, based on what nested route is showing
      appController: controllerInject('application'),
      currentRouteName: reads('appController.currentRouteName'),
      isAssetsIndexRoute: equal('currentRouteName','assets.index'),
    });
  }
})

After

import Controller from '@ember/controller';
import { inject as service } from '@ember/service';
import { inject as controllerInject } from '@ember/controller';

export default Controller.extend({
  screenSize: service(),
  infobarService: service(),
  ... etc.
});

Now I need to apply this to everywhere else in my app that was injecting into controller.setProperties...

Will post back here when I'm done and let you know how it goes

[buschtoens]

You are correct. Slamming a computed property with set onto an instance is deprecated and inefficient. If you absolutely must install a computed property on an instance and not the base class, use defineProperty.

Still, I highly recommend to, if possible, always declare the computed properties on the base class for performance reasons.

[Techn1x]

Thanks for all of the help @pzuraq and @buschtoens , all fixed up now, I was only doing that approach to injection in a few places (to save on having to create a controller file). It didn't occur to me that that usage wasn't supported, since it had always worked in the past.

It just came as a bit of a shock when updating to Ember 3.1 and there was no mention of any deprecations like this in the release blog, and the error that is generated is very cryptic, doesn't mention which property is the problem, which made it far harder to debug.

Sorry for hijacking the thread!

[buschtoens]

More info: emberjs/ember.js#16519 :)

[ianpetzer]

HI there,

I'm just wondering if a codemod was ever created for migrating from the classis ember decorators to the decorators introduced in ember 3.10.

I've trying to upgrade an older app, starting with 3.5 > 3.12, but I have a lot of instances of the old classic decorators using the import:
import {
computed
} from 'ember-decorators/object';