-
-
Notifications
You must be signed in to change notification settings - Fork 204
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
[WIP] Update order-in-*
rules to handle native class syntax
#657
base: master
Are you sure you want to change the base?
[WIP] Update order-in-*
rules to handle native class syntax
#657
Conversation
order-in-*
rules to handle native class syntax
These changes are looking pretty good so far. Excited to get this in. But I'm a bit unclear on what your questions are asking. |
Can you also delete the "Help Wanted" section from the docs for these rules (which I added recently in #669)? |
Finally getting some time to get back to this. let me see if I can clear up my questions When using classic classes, actions were in a hash and with default ordering: const ORDER = [
'service',
'property',
'empty-method',
'single-line-function',
'multi-line-function',
'observer',
'init',
'didReceiveAttrs',
'willRender',
'willInsertElement',
'didInsertElement',
'didRender',
'didUpdateAttrs',
'willUpdate',
'didUpdate',
'willDestroyElement',
'willClearRender',
'didDestroyElement',
'actions',
'method',
]; this hash was the thing being ordered, not the contents inside. I don't think (at least I don't remember) that the actions in that hash got ordered by line number. With native classes, actions are not in a hash and could be considered to be ordered like So my question is, do we sort actions like we do properties (for example) and/or do we also order them on all of their traits - ie single / multiline plus the actions: @action
foo(){
}
@action
bar(){} where This extends into computed properties as well: @computed('a', 'b', 'c')
get foo(){
}
@computed('e, 'f', 'g')
get bar(){} So in my code I have added 2 new sorting groups
Does this make a little more sense? If not you can find me on discord under the same name. |
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.
The new sorting groups sound reasonable to me, although there is slight additional overhead for consumers.
PR looks good but the order-in-*
docs should be updated to reflect these new groups.
@@ -756,5 +1093,35 @@ eslintTester.run('order-in-components', rule, { | |||
}, | |||
], | |||
}, | |||
// { |
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.
Why's this commented out?
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 is temporary to know the valids were still passing while I was working on something else. I wanted to get direction before I moved forward.
@@ -18,6 +18,7 @@ | |||
"test": "jest", | |||
"test:coverage": "jest --coverage", | |||
"test:watch": "jest --watchAll", | |||
"test:debug": "node --inspect node_modules/.bin/jest --runInBand", |
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.
Can we document this somewhere?
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.
Of course! I find this incredibly useful. If you think it's a good addition, I will add it to the README
order-in-*
rules to handle native class syntaxorder-in-*
rules to handle native class syntax
@cdtinney Sorry for the half baked PR. Before I went any further I wanted to get at least some consensus on a direction since native classes kinda changed the reasoning behind the ordering. I will clean up my PR. I have added WIP, something I should have done earlier |
* @param {string} decoratorName The decorator to look for. | ||
* @return {boolean} Whether or not the node has a decorator. | ||
*/ | ||
function hasDecorator(node, decoratorName) { |
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.
Can you move this hasDecorator
function to utils/utils.js
? types.js
is only supposed to be for basic type checks.
@tylerturdenpants Just want to show my support for you doing this 😜 |
I would also like to show support for this 🙂 has there been any progress, or is there anything that can be done to support this? @tylerturdenpants |
This is the very beginning. I'm looking for feedback on a few things:
-Computeds: same thing as the actions.
-tracked?
Fixes #417. Related to #560.