-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
@astrojs/tailwind: simplify, upgrade & fix support for ts config file #6724
Conversation
🦋 Changeset detectedLatest commit: 7540609 The changes in this PR will be included in the next version bump. 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 |
@@ -0,0 +1,5 @@ | |||
--- | |||
'@astrojs/tailwind': major |
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.
Mistake?
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 can't see the history of the file. @TomPichaud is this change intentional? Shouldn't this be a patch
change, considering the PR says "fix"?
If this is a major
, and breaks existing APIs, the changeset should mention a migration path for what's changed.
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've updated this part with what changed and the migration path.
Hey @TomPichaud are you still working on this change? Thanks! |
I think I'm done. Did you read all of my commit ? I was hoping for some review or discussion around this but I'm happy with my work. Maybe someone can review and approve ? I'm not sure about the version as I've made some changes that affect users with no tailwind config, if any ¿? But like I said, Astro is already pushing the user to have a tailwind config file and can do it through the CLI. I'll let you decide. |
+1 for getting this reviewed asap, thanks for the work Tom 🙇🏽 |
@matthewp Any review would be welcome please ? It's working perfectly on all my projects and I really don't mind if you want to change the version's type. |
@TomPichaud Hi guy, are you still dealing with this PR? |
Just waiting for someone to review and approve... |
We usually don't review the code until the PR is ready; meaning merge conflicts are resolved and tests are passing. As for the overall theme of the PR, definitely in favor of simplifying! One thing that's important is HMR. Have you verified that this change still allows Tailwind changes to HMR in the client? Have you verified that changing the config triggers HMR as well (the And just to clarify, the main new feature / bug fix of this PR is that TypeScript and ESM configs are now supported, is that correct? |
I think we should separate the fix thing and refactor thing. To support TypeScript config is high priority, we should deal with it first. And then later we can refactor the tailwind integration at other PR. How do you think? @TomPichaud @matthewp |
Like I said, there's little point doing redundant work. To my opinion it's best to let tailwind load the config and not do it ourself, adding more dependencies, bug possibilities and the necessity to update it when tailwind add a feature. Simple|Less is better, and if we follow tailwind's recommendations we just need to configure postcss + vite, like I'm suggesting we do. Also they are working on automatic 'content' detection so it will already be ready for that. So I disagree on the fix first and then refactor because the fix is adding dependencies as the refactoring is removing some dependencies to get the same result. I suggest you test this pr thoroughly and merge it. I've got no issues locally. |
Yes, I test it locally, it can works. I see the branch has conflicts need be resolved, can you deal with it? |
I've rebased the PR and it should be good to go now. I also think it's nice to clean up the config by moving |
Thanks for your help! |
Let's do a preview release and test it out. In particular we need to verify there are no regressions in terms of HMR, which is something we fixed in the previous implementation. |
!preview tailwind-config |
|
For some reason the preview release didn't work. So I manually did a beta release which you can test with: npm install @astrojs/tailwind@beta |
HMR looks good 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.
Let's 🚢 it!
First participation to Astro! Love your project and looking forward to its evolution!
Fix: #6695
Changes
Commit 8a8be42 : add support for ESM/TS config files while maintaining current behaviour: providing a default tailwind config for astro if none is present.
Knowing that Astro can already suggest and generate a default tailwind config trough the command
astro add tailwind
, I think we can simplify this integration further, reducing its dependencies and making it more robust.In light of the above, I've made the following commit 4a515e3 : only setup postcss, supporting custom config file path and the base css import
Also, I feel like any integration setup should be inside the integration package and not hard coded into astro to be able to use
astro add ...
.Testing
Tested locally with a
tailwind.config.ts
and customcustom-tailwind.config.ts
Docs
No further documentation is needed, but we could add ESM/TS support