Convert classic fields to prototype fields instead of class fields when run against an addon

[elwayman02]

Given the following classic class pattern:

EmberObject.extend({
  foo: 'some value',
  bar: function () {},
  baz: () => {},
  bat() {}
});

The codemod should create the following output when run in addons:

class MyClass extends EmberObject {
  bat() {}
}

MyClass.prototype.foo = 'some value';
MyClass.prototype.bar = function () {};
MyClass.prototype.baz = () => {};

instead of what it creates today:

class MyClass extends EmberObject {
  foo = 'some value';
  bar = function () {};
  baz = () => {};

  bat() {}
}

This must be done to ensure the codemod is making safe migration in case of classes being extended by consumers. That is, if we had the following class in a consuming app:

MyClass.extend({
  foo: 'other value',
  bar() { // some other function },
  baz: () => { // some other function },
  bat() { // some other function }
});

Then the values of foo, bar, and baz in this extending class would be clobbering by their underlying definition within MyClass, where the codemod has made them class fields applied to the instance.

Similarly, if we have a native class like this:

class MyExtendingClass extends MyClass {
  foo = 'some value';

  bar() {};
  baz() {};
  bat() {}
}

Then in this case, both bar and baz would get clobbered, likely causing unexpected bugs as the consumer believed they were overriding the default values in the base MyClass.

Of course, the proposed syntax is rather undesired, and in the long run you would want to find a way to convert these back to class fields after evaluating the risk and communicating with consumers (such as through a breaking release). Since this is far more likely to occur in an addon (which could have many consumers, any of which may be extending classes in ways the author can't predict) than in an app, my proposal is that we have the codemod only make changes in this manner when migrating an addon, and to keep the current behavior in apps.

To maintain the existing behavior, a second codemod could be added to this package that migrates prototype fields to class fields automatically, along with an appropriate warning of the inherent risk. This would allow people to have a much safer migration in the first step, and then optionally choose an aggressive migration to class fields as a separate change that would be easier to reason about and judge the risk level of those changes.

[asakusuma]

@elwayman02 to make sure I understand what you are proposing, you want to:

1. Use the "MyClass.prototype.foo" syntax by default when converting a class in an addon
2. Add an option to use the current behavior instead of the default "MyClass.prototype.foo" behavior

Is that correct? And if so, would you be ok with actually flipping the default so that "MyClass.prototype.foo" syntax is opt it? That way the change is not breaking.

[elwayman02]

@asakusuma making it opt-in behavior doesn't actually make sense, because the current behavior is unsafe by default. If anything, the unsafe behavior should be the opt-in (hence the suggestion of creating a second codemod that can be run to aggressively maintain the current behavior). I don't think this is a breaking change at all, it's a bugfix.