-
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
Handle decorators #5
Conversation
538a99d
to
5873ae1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looking good, some minor comments but I think the structure is looking good. Generally bucketing things into parse and transform helpers seems to be a solid pattern, and config of whether to use decorators or not is 👍
@@ -10,7 +10,7 @@ | |||
"codemod-cli" | |||
], | |||
"dependencies": { | |||
"codemod-cli": "^0.1.0" | |||
"codemod-cli": "ssutar/codemod-cli" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there an upstream PR here? Ideally we should get that merged and published before we merge this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, there is PR rwjblue/codemod-cli#41 Not sure if it will be merged. I'll sync up with @rwjblue and merge it
}); | ||
|
||
// Do not transform as extends Mixin | ||
const Foo = EmberObject.extend(MyMixin, { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be transformed to the following:
class Foo extends EmberObject.extend(MyMixin) {
biz = 'div';
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking of adding mixin support in different PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like you added mixin support, so this test case can be removed/updated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mixin support is added with config option --mixins=true
with codemod command line. Please see the mixins.options.json
. So we need to keep the mixin test case
}); | ||
|
||
// Do not transform | ||
const Foo = EmberObject.extend({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should transform to:
@layout('div')
class Foo extends EmberObject {}
@controller("abc") | ||
myController; | ||
|
||
@observes("xyz") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may want to come back to observers/event listeners. The trickiness is in subclasses, we must apply the opposite if the field is overriden:
const Foo = EmberObject.extend({
fooObserver: observer('foo', function() {})
});
const Bar = Foo.extend({
fooObserver: null, // unsets the observer
});
class Foo extends EmberObject {
@observes('foo') fooObserver() {}
}
class Bar extends Foo {
@unobserves('foo') fooObserver = null;
}
This will mean we have to add a second pass to the codemod, since we can't know what the subclass/superclass is until we've completed the first pass.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the detail, I'll create a new issue so that we can track this separately
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added #7
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, one minor comment about a test case but I think once that's resolved this is good to merge 👍
}); | ||
|
||
// Do not transform as extends Mixin | ||
const Foo = EmberObject.extend(MyMixin, { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like you added mixin support, so this test case can be removed/updated?
/** | ||
* Lname | ||
*/ | ||
@alias.readOnly("firstName", "lastName") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this may need to change to alias('firstName', 'lastName').readOnly()
, but since ember-decorators
doesn't have this API yet we merge this for now and update it later on once that API is finalized
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mixin support is added with config option --mixins=true
with codemod command line. Please see the mixins.options.json
. So we need to keep the mixin test case
Sounds good, thanks again @ssutar! |
This PR handles different decorators mentioned in #1