-
-
Notifications
You must be signed in to change notification settings - Fork 244
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
feat(macro): macro for vite #386
Conversation
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/pmndrs/valtio/HQdZweX8wfZfk67vUsFYq56FokLJ |
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 0588310:
|
rollup.config.js
Outdated
@@ -162,9 +162,13 @@ export default function (args) { | |||
let c = Object.keys(args).find((key) => key.startsWith('config-')) | |||
if (c) { | |||
c = c.slice('config-'.length).replace(/_/g, '/') | |||
if (c === 'macro-vite') { |
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.
Doesn't config-macro_vite
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.
I'll try that (I'm having some issues with types/exports, when importing valtio/macro/vite)
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 mean L164 does the conversion, no?
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.
oh, I thought you mean the cli command? yea, it does! in the end, it's valtio/macro/vite.js
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.
Please remove L165-L167,
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.
remove L171 empty line to to remove the changes in this file.
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.
small change request
rollup.config.js
Outdated
@@ -162,9 +162,13 @@ export default function (args) { | |||
let c = Object.keys(args).find((key) => key.startsWith('config-')) | |||
if (c) { | |||
c = c.slice('config-'.length).replace(/_/g, '/') | |||
if (c === 'macro-vite') { |
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.
Please remove L165-L167,
rollup.config.js
Outdated
@@ -162,9 +162,13 @@ export default function (args) { | |||
let c = Object.keys(args).find((key) => key.startsWith('config-')) | |||
if (c) { | |||
c = c.slice('config-'.length).replace(/_/g, '/') | |||
if (c === 'macro-vite') { |
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.
remove L171 empty line to to remove the changes in this file.
website/.eslintrc.json
Outdated
"root": true, | ||
"extends": "next/core-web-vitals" |
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.
we shouldn't remove this, right??
Co-authored-by: Daishi Kato <dai-shi@users.noreply.github.com>
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.
Looks cool! We have undocumented convention of specifying versions. Please see comments.
Co-authored-by: Daishi Kato <dai-shi@users.noreply.github.com>
Co-authored-by: Daishi Kato <dai-shi@users.noreply.github.com>
Co-authored-by: Daishi Kato <dai-shi@users.noreply.github.com>
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.
Looks good now.
Hi @Aslemammad it seems could you please upgrade vite peer dep verison for the macro plugin?
|
I released v1.0.0, I think it would work now. @Kaijun |
Resolves #299