-
-
Notifications
You must be signed in to change notification settings - Fork 130
Refactoring to include husky pre-commit hooks, improved eslint rules and manual chunking + more #73
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
Conversation
…dividual build file size, updating .gitignore
✅ Deploy Preview for quicksnip ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Really like quicksnip and wanted to help a little. I couldn't find any open issues pertaining to some of the changes that I'm proposing above, but if these are already being addressed then feel free to close this PR. This PR should clean up the codebase a little and make the code more consistent. If there are any questions please ask. Some of the changes are personal preference that crept in and can be ignored/removed if another pattern has already been established. |
… are no unit tests yet. seems as if some of the escapes could be removed in the future
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.
Hey, thanks for your contribution !
I'm not a big fan of the index.ts
everywhere just re exporting stuff... But let's wait on other feedbacks
Hey, there. Thank you for the contribution. Glad you liked our project. Our maintainers will review the PR. Please, it would be great if you first create an issue before making (big) changes in the codebase next time. It would help us understand what you're trying to tackle/build. If we're already working on it, it would save you time as well. Thank you for understanding. |
Hello, thanks for your reply. In future I will create an issue, but I was unsure of whether or not I should have done this at the time as the above code doesn't fix anything - it's more of a quality of life change. PR looks big but it's mostly eslint libs that have been added. |
IMO appart from the barrel imports, everything looks good, and looks like a good improvement to code quality, and as it also configures Eslint a bit more, it will allow for contribution to be more within coding standards in the future |
About the linting and husky parts I believe they are alright, but I am not that experienced with them that much. About the change to barrel imports. I did some research and found that majority of the dev community warns against using barrel imports, even Vite themselves in their documentation. So, only if everyone is on board with that maybe we should move towards it. An alternative would be to use path aliases, which we can define in the |
I am also against barrel imports. It may be fine for something like I also think for consistent imports we should have something like husky and chunks are a nice addition though. But with chunks It might be better to just add node_modules directory into it rather than cherry picking every packages. |
Seems like I'm showing my age with the barrel imports. I've removed them now as they seem to do more harm than good these days. |
Should we add path aliases inside this PR, or maybe do it in a new one? @Mathys-Gasnier @saminjay @dostonnabotov |
I've just added aliases with a new commit. |
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.
For me it looks good now.
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.
LGTM
Awesome work 👍 |
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.
Setting approval for reference
Description
vite-tsconfig-paths
to make imports more consistentType of Change
Checklist
Related Issues
Closes #
Additional Context
Screenshots (Optional)
Click to view screenshots