feat(eslint-config-kodefox): module based import order #400
Conversation
| groups: [ | ||
| 'module', | ||
| ['parent', 'sibling', 'index'], | ||
| ['//use[A-Z].*/', '//helper(s?)/'], | ||
| ['//constant(s?)/', '/type(s?)/'], | ||
| ], |
There was a problem hiding this comment.
Personally, I agree with the parent, sibling and index being on the same level.
But I'm not sure about the separation based on content being baked into the config directly.
As of right now, there's no convention about where to put the hooks. So usually I would put them in the same directory as helpers.
I'm not 100% sure about forcing projects that use this config to match their folder structures based on our personal preferences.
This might work if we specify what kind of project this config is for or put this behind option flag.
There was a problem hiding this comment.
Also, just nit, but if we're talking about directories, I think the name should always be plural, so the s is not optional, cheers.
There was a problem hiding this comment.
Oops, I wrote this like days ago and forgot to submit it.
There was a problem hiding this comment.
As of right now, there's no convention about where to put the hooks. So usually I would put them in the same directory as helpers.
That's why I didn't define any directories for hooks and just define use as everyone usually put it all over the place, could be anywhere.
I'm not 100% sure about forcing projects that use this config to match their folder structures based on our personal preferences.
I see. That is a good point. By their folder structures, do you mean the user outside our company?
There was a problem hiding this comment.
I see. That is a good point. By
their folder structures, do you mean the user outside our company?
Not necessarily, we have variations in the older projects, some use lib and maybe utils
Those projects probably can't make use the order unless we add more name variation in the helpers group.
Maybe I worry too much, probably those projects won't use this config anyway.
There was a problem hiding this comment.
I think so, unless we want add separate config for additional strictness.
Something like extends: kodefox/strict.
There was a problem hiding this comment.
Maybe it's better if we have a github action that will do fix from the PR.
Just write a comment in the PR, /format and then import order will be magically fixed ✨
There was a problem hiding this comment.
I'm not sure there will be any people using that except me 😂
There was a problem hiding this comment.
Just write a comment in the PR,
/formatand then import order will be magically fixed
This is new to me!
I don't think we need this new plugin, right?
I think so, unless we want add separate config for additional strictness.
Agreed, let's skip the additional strictness for now and if we find a need for it in future we can create a way to turn it on.
There was a problem hiding this comment.
This is new to me!
I think this GitHub action doesn't exist yet, we need to make one 😆
EDIT: Someone already made it, we just need to use it
https://github.com/marketplace/actions/pull-request-comment-trigger
| 'module', | ||
| ['parent', 'sibling', 'index'], | ||
| ['//use[A-Z].*/', '//helper(s?)/'], | ||
| ['//constant(s?)/', '/type(s?)/'], |
There was a problem hiding this comment.
Is it intended that you didn't escape the / for types?
There was a problem hiding this comment.
Yes, because sometimes we didn't put types.ts inside types directory.
There was a problem hiding this comment.
And also could be there is a chance that someone using something.types.ts or something.type.ts.
There was a problem hiding this comment.
Usually when a regex is in string form it doesn't have the / on each side (that is a syntax for the regex literal) and thus doesn't need those characters to be escaped since they don't have special meaning. So in this case are the / intended to match actual slashes? In that case why are there double at the beginning? (Maybe this plugin uses a weird form of regex string?)
There was a problem hiding this comment.
From the plugin example, it uses / on each side of the regex string so I follow the pattern.
The reason why there is double / at the beginning is that the second one is intended to match the / from directory path.
This is kinda my proposal for
import order.Currently, our
import orderis based on the path parenthesis. So the default order is frombuiltinthenexternaltheninternalthenparent paththensibling paththenindex.I don't think such order is our usual approach as the we did in the past. Thus, I make this proposal to make the
import orderbased on the module type.So first goes the
modulethenparent/sibling/indexas the same level, thencustom hooks/helpersas the same level, thentype/constantas the same level. In my opinion, that is the most similar approach that we used in the past. Of course, we can tweak the order if there is any suggestion.Here is what happens when we put a
constantfile on top of a component file:And here is the error message:
Lastly, here is the example repo if you want to try it yourself.
https://github.com/oshimayoan/import-order-example