-
Notifications
You must be signed in to change notification settings - Fork 38
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Transforms #1
Comments
TransformsSafe equivalent transformThe equivalent transform of extend would be: Input
|
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. |
Another thing that is different by moving methods to the class body, they become non enumerable. |
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 I think the second form for mixins is best tbh, very clear and better for debugging definitely. |
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)... |
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:
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:
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 |
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);
}
} |
The value is that the intermediate class 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. |
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);
}
} |
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() {}
} |
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? |
@scalvert - I also think that is preferable. Specifically because it means:
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 |
👍 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%. |
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. |
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. |
@pzuraq How do we handle the
and
|
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;
} |
How should we handle the For example
Should it be
@pzuraq ^^ |
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 Unfortunately, we've already diverged in behavior-wise in 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:
I like option 2. We also need to add a |
I like the option #2, so to confirm, we should be adding
But as per option 2, decorators will be |
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. |
@ssutar it would be overridden by a config variable, either in |
@pzuraq How should we transform for the following example
Should it be converted to
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 |
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 |
Thanks @pzuraq ! Which name should we use as decorator -- local or imported? In the above example
OR
I think option 2 is the right one, but would like to know your thoughts. |
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 |
What are the module names from which the decorators should be imported? Are those under Also looking from the examples in the |
They are under 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 |
Closing this issue since it's pretty stale at this point, we can reopen independent issues as needed |
Opening this issue to start tracking the various inputs and outputs of this transform. I think the list I have here is pretty comprehensive so far, let me know if you know any other edge cases or features.
Base
Stage 1
Stage 2
The text was updated successfully, but these errors were encountered: