-
-
Notifications
You must be signed in to change notification settings - Fork 46
feat: add vite.addPlugin
#633
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
🦋 Changeset detectedLatest commit: cfc1102 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
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.
What's the purpose behind this change? As far as I'm aware we are currently only using this in one place (devtools-json) and the current code is smaller and better readable and understandable in my opinion.
But maybe I'm missing what you are trying to achieve here.
Ignore my last comment, just found the issue. let me check then |
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.
Overall this looks good and I understand the changes now. As for the file location, maybe we could do something like /tooling/js/vite
as we already have /tooling/js/kit
as well. Not a huge fan of this, but I think it would be better understandable than what we have right now.
haha, perfect 👍 |
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 really like this! This is way better than the previous implementation, imo. Some minor stuff, but i think we are nearly good to go
Co-authored-by: Manuel <[email protected]>
Gonna review this after #639 is merged to reduce the diff. |
Should now be in a better stage with the other PR merged. Now this PR is focusing on vite.
I like a lot this repo :) |
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.
Thank you!
fix #632
I just created a
helper.ts
& some tests, but it should probably move to another location ?Maybe
addInArrayOfObject
&exportDefaultConfig
could go incommon
?And
addPluginToViteConfig
is maybe a bit higher level ? Or it could go to common as well ?I did the demo on
devtool
plugin, and the API looks nice! no ? @benmccann @manuel3108 let me know what do you think.