Skip to content
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

Add support for Tailwind #11717

Merged
merged 3 commits into from
Dec 14, 2021
Merged

Add support for Tailwind #11717

merged 3 commits into from
Dec 14, 2021

Conversation

iansu
Copy link
Contributor

@iansu iansu commented Dec 5, 2021

Load the Tailwind PostCSS plugin if the project includes a tailwind.config.js file

@iansu iansu added this to the 5.0 milestone Dec 5, 2021
@iansu iansu requested a review from mrmckeb December 5, 2021 23:38
@iansu
Copy link
Contributor Author

iansu commented Dec 5, 2021

Looks like tests are failing due to changes in package-lock.json. I committed that file though. @lukekarrys any idea what might be happening here?

@adamwathan
Copy link

@iansu Thanks so much for getting the ball rolling on this, so many people are going to be super stoked to be able to use Tailwind with CRA in such a clean way!

The thing that stands out the most to me is that CRA includes postcss-normalize in the PostCSS plugin list, and Tailwind includes it's own reset/base styles based on Normalize, so it's redundant and will lead to a lot of duplicate/unnecessary CSS being included.

Personally I really believe that the best solution to all of this is to allow people to provide their own postcss.config.js file, and if present have that override the PostCSS config that CRA includes out of the box (so no magic merging or anything like that).

Then CRA wouldn't need anything specific to Tailwind in the codebase at all, and people could use whatever PostCSS plugins they want. I think PostCSS plugins are very different territory than letting someone customize the webpack config or anything like that personally, and I think it would be an extremely low risk thing to support.

Then the path to adding Tailwind would be exactly what's in our docs for other tools:

npm i -D tailwindcss postcss autoprefixer
npx tailwindcss init -p

Done 🥳

If you're really opposed to supporting PostCSS plugins directly then I think the solution you've started on here is the next best thing, but I would change the plugin order to this:

tailwindcss
postcss-flexbugs-fixes
postcss-preset-env

...and not include postcss-normalize when Tailwind is enabled. In general I think this is still not ideal though, because postcss-preset-env is extremely heavy handed and things like their nesting support for example doesn't play very nicely with Tailwind.

In my own personal projects, the only plugins I would use are:

postcss-import
tailwindcss
autoprefixer

...so in a perfect world you'd be able to do that with CRA.

@adamwathan
Copy link

Just tested out the next tag and this seems like it actually already works? I created a postcss.config.js file and CRA picked that up. Is this a happy accident? Hah.

@oxvijay
Copy link

oxvijay commented Dec 10, 2021

@adamwathan @iansu Thanks for your great work. You released Tailwind 3 and suggested using CRA 5 Alpha. But it seems TW3 is not integrated fully yet. Can we use it in our development project? When will this be ready for production?

@padorang684
Copy link

Tried creating a fresh project with react-scripts@next and tailwind@^3, set up tailwind following the steps on https://tailwindcss.com/docs/installation/using-postcss. Tailwind worked perfectly out-of-the-box without any customisations through craco, which was required in previous react-scripts versions.

@raix raix mentioned this pull request Dec 12, 2021
@iansu
Copy link
Contributor Author

iansu commented Dec 12, 2021

@adamwathan We can have a totally separate list of plugins for Tailwind. What about something like this:

plugins: !useTailwind
  ? [
      'postcss-flexbugs-fixes',
      [
        'postcss-preset-env',
        {
          autoprefixer: {
            flexbox: 'no-2009',
          },
          stage: 3,
        },
      ],
      // Adds PostCSS Normalize as the reset css with default options,
      // so that it honors browserslist config in package.json
      // which in turn let's users customize the target behavior as per their needs.
      'postcss-normalize',
    ]
  : [
      'tailwindcss',
      'postcss-flexbugs-fixes',
      [
        'postcss-preset-env',
        {
          autoprefixer: {
            flexbox: 'no-2009',
          },
          stage: 3,
        },
      ],
    ],

Is there anything you would change in the list of Tailwind plugins? You mentioned postcss-import and autoprefixer. Should those be included?

@adamwathan
Copy link

@iansu Good to know! Did you catch my comment about how it seems like the approach I would actually want to use already works in the next branch? Adding a postcss.config.js file seems to automatically override the one used by CRA internally.

Here's a repo:

https://github.com/adamwathan/tailwind-cra-next

So it looks like you don't actually have to do anything, Tailwind will just work in the next version of CRA.

@iansu
Copy link
Contributor Author

iansu commented Dec 12, 2021

@adamwathan That config loading behaviour was unexpected. Something must have changed in postcss-loader. I don't think we want to open that up at this point which is why I made the suggestion above.

@adamwathan
Copy link

I would really recommend just leaving it working as-is, there's no danger in it and then you don't need any Tailwind specific code in CRA at all.

@iansu
Copy link
Contributor Author

iansu commented Dec 13, 2021

@adamwathan I've discussed this with the other maintainers and we've decided not to open up the PostCSS config. Our main concern is that misconfiguration will cause issues for people which will then lead to more issues opened in Create React App. This project has very limited resources and we already can't keep up with issues and this is something we're always trying to balance.

I've updated this PR and would appreciate your feedback on it before we move ahead.

@iansu
Copy link
Contributor Author

iansu commented Dec 13, 2021

I guess Tailwind 3 just came out too. Does that change anything here?

Copy link
Contributor

@raix raix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only had a minor comment - seems Tailwind 3 documentation just states that it should be the top/first loader.

@@ -68,6 +68,11 @@ const imageInlineSizeLimit = parseInt(
// Check if TypeScript is setup
const useTypeScript = fs.existsSync(paths.appTsConfig);

// Check if Tailwind config exists
const useTailwind = fs.existsSync(
path.join(paths.appPath, 'tailwind.config.js')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe move it to paths.appTailwindConfig?

@iansu
Copy link
Contributor Author

iansu commented Dec 13, 2021

@raix Tailwind is the first loader: https://github.com/facebook/create-react-app/pull/11717/files#diff-8e25c4f6f592c1fcfc38f0d43d62cbd68399f44f494c1b60f0cf9ccd7344d697R162

@iansu iansu merged commit 5614c87 into facebook:main Dec 14, 2021
@iansu iansu deleted the tailwind branch December 14, 2021 05:47
@adamwathan
Copy link

I've updated this PR and would appreciate your feedback on it before we move ahead.

Hey, hope this is seen even though the PR is merged!

Personally if the plugins cannot be edited by the user then I would recommend this as the sort of "blessed" official Tailwind PostCSS plugin list:

[
  'postcss-import',
  'tailwindcss/nested',
  'tailwindcss',
  'autoprefixer',
]

I would not recommend postcss-preset-env with Tailwind because the features are all for people who author a lot of CSS, and in Tailwind projects you almost never author CSS at all. It's nesting support also introduces conflicts with Tailwind (that's why we had to create our own nesting plugin), and because of the new PostCSS visitor API you can have issues where things run out-of-order which can corrupt the input Tailwind receives as well.

I'm not really sure what the flexbugs plugin does — we don't use this on any of our own projects and haven't had any issues so I hesitate to recommend it alongside Tailwind.


@adamwathan I've discussed this with the other maintainers and we've decided not to open up the PostCSS config. Our main concern is that misconfiguration will cause issues for people which will then lead to more issues opened in Create React App. This project has very limited resources and we already can't keep up with issues and this is something we're always trying to balance.

I hate having these conversations in public because it feels like a stage and combative, but I really think it would be better for everyone (including the maintainers) if you just let postcss-loader do what it does by default so people can have control of the PostCSS config. By taking that approach, you have less code to maintain here, you don't have to collaborate with me when something changes with CRA or Tailwind that involves you needing to make changes to the hard-coded implementation in CRA, and users will be less frustrated that they don't have control over what features they are using in their projects.

Changes that people make to their PostCSS config are totally isolated to just the CSS which makes them low risk, and any changes they make are not really your responsibility.

I would bet that by locking down the PostCSS config you are actually increasing the odds of issues opening up, because now there are more things that people can't do without having to ask you to do more work for them.

CRA has 1100 open issues and 287 open pull requests, so it is going to be impossible to ever get that stuff under control anyways — at this point I think it's not worth feeling pressure to keep up with them because you simply never will be able to.

My recommendation would be to rework your issue templates to discourage opening issues for things that are really support requests — here's how ours look on our projects:

https://github.com/tailwindlabs/tailwindcss/issues/new/choose

image

We deliberately don't use actual issue templates to ensure the "Bug Report" is not the first item on the list, and instead make "Get Help" the first item which redirects to starting a new GitHub discussion.

We also use the "Convert to Discussion" feature aggressively, so that the only open issues are things we truly believe are issues. Once something is in discussion land, it's not on the maintainers to reply (although we often do) and instead just a place for people to talk amongst each other about how to deal with a problem they've run into. This is the only sustainable approach I have found for managing this stuff after working full time on it for several years now leading very popular projects. It's simply not possible for a small team of volunteers to deal with every single piece of inbound communication.

I really doubt CRA actually has 1100 bugs right, it's just an indication of how hard it is to manage the waterfall of questions coming in from users.

All that to say letting people edit their PostCSS config cannot create any actual bugs in CRA, and therefore cannot actually create any real issues, only questions, which are better moved to the community discussion area anyways.

The team is of course free to do whatever you want, it's your project — I don't want to come across like I'm offering anything other than candid advice as a fellow maintainer who has been extremely stressed out by the problem of issue management for a long time ❤️

(This all ties back in to the original Twitter conversation about why I want to recommend just using our CLI with npm-run-all or something though, because as is evident here we disagree on how this should work, and the only way for me to guarantee a good developer experience to our own users is to recommend an option that we have total control over.)

@@ -69,6 +69,7 @@
"semver": "^7.3.5",
"source-map-loader": "^3.0.0",
"style-loader": "^3.3.1",
"tailwindcss": "^3.0.2",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tailwind shouldn't be an optional dependency? like sass.

Copy link

@oliviertassinari oliviertassinari Jun 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a dedicated issue about this point #12044.

@iansu
Copy link
Contributor Author

iansu commented Dec 14, 2021

Thanks for the feedback Adam. I will make a followup PR to implement the list of plugins you suggest.

I don't necessarily disagree with you about opening up the PostCSS config being relatively low risk. I would say that option isn't off the table. It's a lot easier to go from where we are now to opening up the config than it is to go the other direction. I'll give this some more thought and discuss it with the other maintainers. It's possible we could do this in a minor release. We are also considering some larger changes to Create React App in a future major version (nothing concrete, just exploring ideas right now). I really appreciate your thoughtful feedback on this subject.

We definitely have so many open issues at this point that it's impossible to keep up. We were doing bi-weekly triage meetings for a while but got distracted trying to get v5 released. Issue templates is something I've been meaning to implement for a while. I also like how you're trying to steer people towards discussions and converting non-issues into discussions. Definitely some stuff we can learn from you and the Tailwind team there. Thanks again and I will almost certainly follow up with you on some of these things in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants