-
-
Notifications
You must be signed in to change notification settings - Fork 601
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
refactor: config group to pure function and move away from group design #1811
Conversation
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.
Let's keep entry default option
It's unused |
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.
Also default value for entry
is invalid, so 👍
We need to run ConfigGroup needless whether config args are present or nay, I think we should make it a pure function, this should be a good start to move away from group design. Thoughts? @evilebottnawi @evenstensberg |
Agree, we don't need group design and run only necessary code |
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.
Good job 🔥
a1bf24d
2c17d34
to
a1bf24d
Compare
@anshumanv Thanks for your update. I labeled the Pull Request so reviewers will review it again. @snitin315 Please review the new changes. |
|
||
module.exports = ConfigGroup; | ||
module.exports = handleConfigResolution; |
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 we need move this from ConfigGroup.js
file, to be honestly we don't need groups
files 😄 Do you want to do it in this 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.
Yep lets do it here
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.
Or maybe let's migrate all groups first then we can move them all from groups
to argResolvers
?
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.
Maybe we put each function in own file, anyway let's refactor firstly and when we can think about places
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.
yep once all groups are migrated, we can organize it neatly
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.
Good start
What kind of change does this PR introduce?
refactor
Did you add tests for your changes?
Already present
If relevant, did you update the documentation?
NA
Summary
Does this PR introduce a breaking change?
No
Other information
#1765 (comment)