Skip to content
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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tylerturdenpants
Copy link

@tylerturdenpants tylerturdenpants commented Jan 31, 2020

This is the very beginning. I'm looking for feedback on a few things:

  • Actions: in classic classes actions were in a hash, but in native classes they aren't. Should they be ordered by line count as well?

-Computeds: same thing as the actions.

-tracked?

Fixes #417. Related to #560.

@bmish bmish changed the title Order-in-* rules in native class syntax Update order-in-* rules to handle native class syntax Feb 9, 2020
@bmish
Copy link
Member

bmish commented Feb 9, 2020

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.

@bmish
Copy link
Member

bmish commented Feb 14, 2020

Can you also delete the "Help Wanted" section from the docs for these rules (which I added recently in #669)?

@tylerturdenpants
Copy link
Author

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 method property because there can be any number of them.

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 bar and foo would be not only ordered because they where actions, but that bar could be configured to come before foo?

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 action-single-line-function and action-multi-line-function, to try and capture the other traits.

const ORDER = [
  ...
  'willClearRender',
  'didDestroyElement',
  'action-single-line-function',
  'action-multi-line-function',
  'actions',
  'method',
];

Does this make a little more sense? If not you can find me on discord under the same name.

Copy link
Contributor

@cdtinney cdtinney left a 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, {
},
],
},
// {
Copy link
Contributor

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?

Copy link
Author

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",
Copy link
Contributor

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?

Copy link
Author

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

@tylerturdenpants tylerturdenpants changed the title Update order-in-* rules to handle native class syntax [WIP] Update order-in-* rules to handle native class syntax Feb 14, 2020
@tylerturdenpants
Copy link
Author

@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) {
Copy link
Member

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.

@Alonski
Copy link
Contributor

Alonski commented Feb 17, 2021

@tylerturdenpants Just want to show my support for you doing this 😜

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Order-in-* rules do not work in native class syntax
4 participants