Another thing that is different by moving methods to the class body, they become non enumerable.

from ember-native-class-codemod.

Yes, we definitely want to keep any local names that have been applied. Decorators should match up 1-1, so we can transform the import statements and everything should work

from ember-native-class-codemod.

Transforms

Safe equivalent transform

The equivalent transform of extend would be:

Input some-class.js

export default EmberObject.extend(MixinA, MixinB, {
  // ... EmberObject.extend stuff ...
});

Output some-class.js

export default class SomeClass extends EmberObject {}
SomeClass.reopen(MixinA, MixinB, {
  // ... EmberObject.extend stuff ...
});

• transform const [Subclass] = [Baseclass].extend(...args) call to class Subclass extends Baseclass {}
  • infer class name from file if default export otherwise use the variable name or export name.
• move extend args to [Subclass].reopen(...args)

The benefit of the above is debugging and constructor.name.

It is the exact equivalent of what CoreObject.extend is currently.

Unsafe transform

Move methods outside of extends pojo into class body and change _super() to super.methodName()

To do this safely, either there are know mixins in the args to extend or
you know what is in the mixins and whether or not they contain mixins that
have already been applied. Otherwise the mixin applied in reopen might have
a method that wraps the method already in the class prototype from the class body.

Input some-class.js

export default EmberObject.extend(MixinA, MixinB, {
  someMethod(x, y) {
    this._super(x, y);
  },
  // ... non function extend stuff ...
});

Output some-class.js

export default class SomeClass extends EmberObject {
  someMethod(x, y) {
    super.someMethod(x, y);
  }
}
SomeClass.reopen(MixinA, MixinB, {
  // ... non function extend stuff ...
});

Unfortunately if MixinA or MixinB have someMethod and that method hasn't already been applied to a superclass's prototype
the super ordering is will be wrong since the mixins will wrap the method in the class body instead of the other way around.

Calling super of a mixins method in the same prototype requires wrapping. In ES classes super always refers to the prototype.

So an alternative, is to create an extra prototype just for the mixins (which is not needed unless there is a super issue).

Alternative some-class.js

export default class SomeClass extends EmberObject.extend(MixinA, MixinB) {
  someMethod(x, y) {
    super.someMethod(x, y);
  }
}
SomeClass.reopen({
  // ... non function extend stuff ...
});

Now this should be a safe transform, though it is not equivalent.
The extra prototype in general should be ok.

Alternative is not to move methods that have super if there are mixins and those mixins can not be resolved statically.

The benefit of this is it removes the need for _super wrapping (outside of mixins) which greatly
improves debugging and likely performs better.

from ember-native-class-codemod.

Another issue with moving functions is maybe the function is intended to be a property whose value is a function and not a method.

Also, another alternative form for the extra mixin prototype example above is:

class SomeClassMixins extends EmberObject {}
export default class SomeClass extends SomeClassMixins {
  someMethod(x, y) {
    super.someMethod(x, y);
  }
}
SomeClassMixins.reopen(MixinA, MixinB);
SomeClass.reopen({
  // ... non function extend stuff ...
});

This maybe adds some debuggability in that it gives the extra prototype a name.

from ember-native-class-codemod.

How would we distinguish between a function that is a method and one which is a property that’s meant to be a method? Unless the function is passed to create it would end up on the prototype either way, I’d think, so should essentially be equivalent

I think the second form for mixins is best tbh, very clear and better for debugging definitely.

from ember-native-class-codemod.

Yes, I’m also in favor of this form:

export default class SomeClass extends EmberObject.extend(MixinA, MixinB) {
  someMethod(x, y) {
    super.someMethod(x, y);
  }
}
SomeClass.reopen({
  // ... non function extend stuff ...
});

The caveats with the other form (adding Mixins in a reopen after the class) just seem too likely to be massive footguns. I’m also fine if we decide not to convert anything with Mixins in the first pass (and therefore avoid this issue)...

from ember-native-class-codemod.

I was actually thinking the form where we do reopen the intermediary mixin class, so that you could see in the debugger the class name:

class SomeClassMixins extends EmberObject {}
SomeClassMixins.reopen(MixinA, MixinB);

export default class SomeClass extends SomeClassMixins {
  someMethod(x, y) {
    super.someMethod(x, y);
  }
}

I do think this is not the option most users will end up using in their own code though. Maybe we can make this configurable?

Either way, talking with Kris more, I think we should be adding more stages here. I'm going to amend the list above with his suggestions, here's what I'm thinking for overall stages and intent:

• Stage 0: This stage's goal is to convert, but to try to make sure that the final class/prototype is as close to the original as possible. It will move the majority of class definition to reopen, and will be the safest transform. This brings some ergonomic benefits for things like debugging, but none of the class benefits themselves. This transform can be applied to all classes.

• Stage 1: This stage will transform any classes that do not require decorators, and leave others alone (at stage 0). The transform will be changing the actual behavior of prototypes, but the goal is to keep their external behavior equivalent. There are subtle changes (enumerability of methods, assignment of class fields) that we can't guard against, and will have to warn about in the Caveats section.

• Stage 2: Converting with decorators, which should be able to transform all remaining classes. This stage will be the most different from the original prototype, because of the way decorated fields are declared and assigned vs the original descriptor pattern. For the most part we should be able to do a 1-1 conversion, but we'll need to tease out caveats and edge cases and either throw warnings/errors, or let users know (in particular thinking about @action and the way it doesn't move action methods)

My thinking for why we shouldn't partially convert classes that have computeds/observers/etc in Stage 1 is because ergonomically, we want to avoid splitting class bodies. I can see this not being terrible - it essentially groups all your computeds and other constructs in a reopen - so open to it, but I think we should think through the repercussions. We could also make it configurable - strict mode where you don't convert anything, looser mode where you convert as much as you can.

from ember-native-class-codemod.

What is the value in this form:

class SomeClassMixins extends EmberObject {}
SomeClassMixins.reopen(MixinA, MixinB);

export default class SomeClass extends SomeClassMixins {
  someMethod(x, y) {
    super.someMethod(x, y);
  }
}

Over the (IMHO) more idiomatic ES version:

export default class SomeClass extends EmberObject.extend(MixinA, MixinB) {
  someMethod(x, y) {
    super.someMethod(x, y);
  }
}

from ember-native-class-codemod.

The value is that the intermediate class SomeClassMixins gets an actual class/prototype name, so you'll be able to see it in debugging/memory debugging. Agreed that it isn't really that much of a benefit though, it's better than the status quo technically speaking, but much less readable.

The tradeoff is between better debugging than today (keeping in mind that users are already used to having "anonymous" class names constantly) and better code comprehension and readability. I think choosing the later as the default is preferable, and possibly including the former as a configurable option, makes the most sense.

from ember-native-class-codemod.

This discussion seems to imply that we'll have an intermediate state, which we'd then need to perform another transformation over to get to the eventual end state. Is this correct?

I do like the added debuggability of this form:

class SomeClassMixins extends EmberObject {}
SomeClassMixins.reopen(MixinA, MixinB);

export default class SomeClass extends SomeClassMixins {
  someMethod(x, y) {
    super.someMethod(x, y);
  }
}

But worry that introduces another non-standard for of object model form. I'd completely agree with moving to the idiomatic version.

export default class SomeClass extends EmberObject.extend(MixinA, MixinB) {
  someMethod(x, y) {
    super.someMethod(x, y);
  }
}

from ember-native-class-codemod.

Right, so there are stages that we can apply to transforming a class, and theoretically they can be applied to all classes. Whether or not we should support a given stage is the open question, I can see value in supporting all of them and I think making the codemod be able to accept any of them as input and move them to the next one would be ideal.

Here's an example of a class going through all the stages. Each stage is defined mainly by the amount of tolerance you have for risk, either for differences in behavior (class fields, decorators, methods, etc. ultimately work differently than the old way) or using experimental language features.

// Input
const Foo = EmberObject.extend({
  prop: 'defaultValue',
  
  method() {},

  get accessor() {},

  computed: computed({
    get() {},
    set() {},
  }),
});

// Stage 0
// Just reopen, very similar end prototype to original, 
// least likely to cause regressions
class Foo extends EmberObject {}
Foo.reopen({
  prop: 'defaultValue',
  
  method() {},

  get accessor() {},

  computed: computed({
    get() {},
    set() {},
  }),
});

// Stage 1
// Convert with ES2015 classes, minor differences in
// behavior, unlikely to cause regressions
class Foo extends EmberObject {
  method() {}

  get accessor() {}
}

Foo.reopen({
  prop: 'defaultValue',
  
  computed: computed({
    get() {},
    set() {},
  })
});

// Stage 2
// Convert with fields and decorators, largest
// differences in behavior of fields and decorators,
// most likely to cause regressions and bugs
class Foo extends EmberObject {
  prop = 'defaultValue';

  method() {}

  get accessor() {}

  @computed
  get computed() {}
  set computed() {}
}

from ember-native-class-codemod.

I guess the idea I had was that we'd be grouping these stages by groupings rather than by phases. What I mean is, we'd simply skip specific Ember objects that were not included in the stage based on that stage's changes.

So, in the first stage, we'd only convert very simple shapes of objects. This would then allow us, in successive stages, to convert more complicated objects that included properties that required a corresponding decorator, etc. This would ensure that we don't have a 'weird' intermediate state while we transform codebases.

Thoughts?

from ember-native-class-codemod.

@scalvert - I also think that is preferable. Specifically because it means:

• the codebase doesn't have odd intermediate states that have to be redone (and taught 🙀)
• a linting rule can be used to enforce that new code must conform to our requirements, the rule can basically have settings ("simple method only objects", "simple objects with mixins", "objects needing decorators", etc)

IMHO, teaching some of the intermediate stages above will be fairly hard. For example, I don't really think developers should need to internalize that .extend() is "just" class Foo extends Thing {}; Foo.reopen({}). Heck it is my personal opionion that we should never teach .reopen 😜...

from ember-native-class-codemod.

👍 then we basically have two possible stages again, with class fields and decorators, and without.

I think the best way to address @krisselden's concerns regarding compatibility then will be a very large, bolded, front-and-center list of all the caveats each stage comes with, and in general things that can go wrong. The goal isn't to preserve the exact same behavior or prototypes, etc, but to have the same end behavior in 99% of cases, and warnings about the remaining 1%.

from ember-native-class-codemod.

I thought the idea behind stages was loose vs most compatible, the idea was people could try to move their app to the most ideal form, and back off. I don't think the extra prototype or non enumerability of methods should cause any real issue but they might.

from ember-native-class-codemod.

I think it would serve us better to minimize the intermediate state as much as possible. I think we should be attempting to convert things in single steps as much as possible. This will ensure code doesn't languish in an in-between state for too long. Additionally, it will be more work for us if we have to convert from the starting state to the intermediate state, then the intermediate state to the end state.

This is particularly true for large codebases, where there's likely to be lots of permutations we have to consider.

from ember-native-class-codemod.

@pzuraq How do we handle the attributeBindings and classNameBindings below:

export default Component.extend({
  tagName: 'a',
  attributeBindings: ['customHref:href'],

  customHref: 'http://emberjs.com'
});

and

export default Component.extend({
  classNameBindings: ['isEnabled:enabled:disabled'],
  isEnabled: false
});

from ember-native-class-codemod.

We split the string, first value is the key we match it up with, rest we pass into the decorator:

@tagName('a')
export default FooComponent extends Component {
  @attribute('href') 
  customHref = 'http://emberjs.com';
}

export default FooComponent extends Component {
  @className('enabled', 'disabled')
  isEnabled = false;
}

from ember-native-class-codemod.

How should we handle the computed property macros

For example

import { or } from '@ember/object/computed';

const Foo = EmberObject.extend({
  cp: or('prop1', 'prop2').readOnly(),
});

Should it be

import { or } from '@ember/object/computed';

const Foo = EmberObject.extend({
  @readOnly // Not sure about this,
  @or('prop1', 'prop2') // OR or('prop1', 'prop2').readOnly()
  cp,
});

@pzuraq ^^

from ember-native-class-codemod.

This opens up a pretty big can of worms w/regards to clobbering 😕

Most of the existing macros are generally only getters, they do not have a setter. In theory, this means that they can all be marked readOnly by default. However, the current behavior of computed properties is that if they do not have a setter, and are not readOnly, then setting the property will clobber: it'll remove the computed property entirely, and set the property to the value.

Unfortunately, we've already diverged in behavior-wise in ember-decorators with the @computed decorator. It does not allow clobbering because clobbering is at odds with the way native getters and setters work, and we were trying to align the two. Clobbering is also a pretty problematic behavior in general, it's very magical behavior that in my experience devs don't really even know exists, and oftentimes stumble into (which causes them to wonder why their computeds break).

This means that the following would break:

// before
const Foo = EmberObject.extend({
  aliasedProp: 123,

  oneWayAlias: computed({
    get() {
	  return this.aliasedProp;
    }
  })
});

let foo = Foo.create();

console.log(foo.oneWayAlias); // 123

foo.set('oneWayAlias', 456);

console.log(foo.aliasedProp); // 123
console.log(foo.oneWayAlias); // 456

// after
class Foo extends EmberObject {
  aliasedProp = 123;

  @computed
  get oneWayAlias() {
    return this.aliasedProp;
  }
}

let foo = Foo.create();

console.log(foo.oneWayAlias); // 123

foo.set('oneWayAlias', 456); // errors

I'm unsure how often this functionality is used in the wild, but there is no easy way for us to detect it since it's very dynamic.

I think there are three viable paths forward here:

1. Change the behavior of @computed to match Ember.computed. Remove the assertion, and deal with clobbering being a possibility until it is deprecated in Ember proper.
2. Make it configurable. Add a flag that makes decorators readOnly by default, but allows this to be disabled. This could be added as an optional feature to Ember later on, and RFC'd as a deprecation.
3. Force it, and let errors happen. I don't think this is an option, given we want codemods to work and not break things.

I like option 2. We also need to add a readOnly modifier to the decorator (@or.readOnly()) functions if we go this route, the combinatorial decorators (@readOnly @or) are problematic for a number of reasons.

from ember-native-class-codemod.

I like the option #2, so to confirm, we should be adding readOnly in the decorators. Like below:

import { or } from '@ember/object/computed';

const Foo = EmberObject.extend({
  @or.readOnly('prop1', 'prop2')
  cp,
});

But as per option 2, decorators will be readOnly by default, do we need the readOnly above? How the readOnly behavior will be overridden?

from ember-native-class-codemod.

Good call out, @pzuraq. I'm also inclined to try to steer people towards predictable behavior while still providing an escape hatch, so to speak. I'd tend to agree with you that option 2 will satisfy both.

from ember-native-class-codemod.

@ssutar it would be overridden by a config variable, either in config/environment.js or, ideally, in ember-cli-build.js as a build step. For use with this codemod, we would either document the flag thoroughly so they know to turn it on, or try to do it for them, so computeds would be assumed to be clobberable, and we would add the readOnly modifier like in your example.

from ember-native-class-codemod.

@pzuraq How should we transform for the following example

import { sum as add } from "@ember/object/computed";

const Foo = EmberObject.extend({
  num1: 0,
  num2: 0,

 addition: add("num1", "num2").volatile()
});

Should it be converted to

import { sum as add } from "@ember/object/computed";

const Foo = EmberObject.extend({
  num1: 0,
  num2: 0,

 @add.volatile("num1", "num2") // Option 1
 @sum.volatile("num1", "num2") // Option 2
 addition,
});

Which one should be used as decorators - local name or imported specifier name?

This brings another question about imports - do we need to import decorators when we use them? Ref: https://medium.com/build-addepar/es-classes-in-ember-js-63e948e9d78e

from ember-native-class-codemod.

We do need to import decorators for sure. For volatile behavior, I think we need to address that in ember-decorators as well. It seems counter intuitive to have to include it, but there are actually subtle differences between native getters/setters and volatiles (volatiles never trigger change notifications, even when set). Will get back to you on that

from ember-native-class-codemod.

Thanks @pzuraq !

Which name should we use as decorator -- local or imported?

In the above example sum is imported and used as add(The reason might be something else with the same name sum is in the local scope). So when we migrate this using decorators I think of 2 options.

import { sum } from "@ember/[some/decorators/path]";

const Foo = EmberObject.extend({
  num1: 0,
  num2: 0,

 @sum("num1", "num2") // Option 1
 addition,
});

OR

import { sum as add } from "@ember/[some/decorators/path]";

const Foo = EmberObject.extend({
  num1: 0,
  num2: 0,

 @add("num1", "num2") // Option 2
 addition,
});

I think option 2 is the right one, but would like to know your thoughts.

from ember-native-class-codemod.

What are the module names from which the decorators should be imported? Are those under @ember-decorators/ or they will be under @ember itself?

Also looking from the examples in the @ember-decorators - Is it possible to have multiple decorators for a single property? The latest PR does not handle that case, but it should be fairly easy to modify

from ember-native-class-codemod.

They are under @ember-decorators, as they aren’t yet part of the official Ember API. To become part of it, they’ll need to be RFC’d into Ember, and we’ve been holding off on that until decorators reach stage 3 in TC39 at least.

It is definitely possible for multiple decorators to apply to a prop, but I’m pretty sure it wouldn’t happen often. The only case I can think of is users can combine on and observer

from ember-native-class-codemod.

Thanks @pzuraq

Added issues #8 and #9 to track these

from ember-native-class-codemod.

Closing this issue since it's pretty stale at this point, we can reopen independent issues as needed

from ember-native-class-codemod.