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

Refactor tailwind integration setup #5717

Merged
merged 4 commits into from Jan 5, 2023
Merged

Refactor tailwind integration setup #5717

merged 4 commits into from Jan 5, 2023

Conversation

bluwy
Copy link
Member

@bluwy bluwy commented Jan 2, 2023

Changes

As discussed in the maintainers chat, the style.postcss Astro config can be removed in favour of vite.css.postcss. They do the same thing internally, and is only a property used by the tailwind integration.

I also disabled autoprefixer in dev. I'm not sure if this was intentional, but enabling it slows down dev a lot so I changed that in the PR too.

Testing

It doesn't seem like there's existing tests for tailwind, but I've tested manually with the with-tailwindcss example and it works.

Docs

n/a. internal refactor.

@changeset-bot
Copy link

changeset-bot bot commented Jan 2, 2023

🦋 Changeset detected

Latest commit: 13387d4

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

@github-actions github-actions bot added pkg: astro Related to the core `astro` package (scope) pkg: svelte Related to Svelte (scope) pkg: integration Related to any renderer integration (scope) labels Jan 2, 2023
@bluwy
Copy link
Member Author

bluwy commented Jan 3, 2023

Marking as draft. This depends on #5685

@bluwy bluwy marked this pull request as draft January 3, 2023 14:00
@github-actions github-actions bot removed the pkg: svelte Related to Svelte (scope) label Jan 4, 2023
@bluwy bluwy marked this pull request as ready for review January 4, 2023 09:38
Copy link
Member

@Princesseuh Princesseuh left a comment

Choose a reason for hiding this comment

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

Looks good to me! I fear a little bit that some users will get confused by autoprefixer not being on in dev, but if we communicate it enough it'll be fine, I think

@bluwy bluwy requested a review from a team as a code owner January 4, 2023 15:35
@bluwy
Copy link
Member Author

bluwy commented Jan 4, 2023

Good point. I've updated the tailwind readme to reflect that. It also doesn't look like we mention autoprefixer anywhere before, so this seems like a good time to bring up. cc @withastro/maintainers-docs for review!

packages/integrations/tailwind/README.md Outdated Show resolved Hide resolved
Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca>
@bluwy bluwy added the semver: major Change triggers a `major` release label Jan 5, 2023
@bluwy bluwy merged commit a3a7fc9 into main Jan 5, 2023
@bluwy bluwy deleted the tailwind-refactor branch January 5, 2023 08:02
return {
css: {
postcss: {
plugins: postcssPlugins,

Choose a reason for hiding this comment

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

@bluwy Setting an inline PostCSS config in Vite, has the effect that Vite no longer looks for postcss.config.cjs.

See:

Note if an inline config is provided, Vite will not search for other PostCSS config sources.
https://vitejs.dev/config/shared-options.html#css-postcss

That results in by adding the Tailwind Integration 3.0.0-beta.x, postcss.config.cjs no longer work as described in the Astro Docs.
https://docs.astro.build/en/guides/styling/#postcss

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm that's true, but I think that happens before this PR too as Astro was assigning to the property internally:

https://github.com/withastro/astro/pull/5717/files#diff-025f8338e5be413b0f65c74bc50d6484c6bc9bb4f78847da0ba896b8e9569390L153-L155

I guess for the tailwind integration we need to read postcss.config.js too to resolve this.

Copy link

@LexSwed LexSwed Jan 22, 2023

Choose a reason for hiding this comment

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

Confirming, trying latest @beta packages makes Tailwind to ignore postcss.config.js (f.e. to enable nesting)

Seem to be addressed in: #5908

Copy link
Member

Choose a reason for hiding this comment

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

Seem to be addressed in: #5908

Came up with a possible fix for the issue, but I'm hitting another issue. Applying changes to the postcss.config.js file only has effect after restarting the dev server.

I just wanted to know if it was an issue before, so that it will get addressed in another PR and doesn't block the current one.

Any ideas?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it was looking at the old code. I'd be fine merging it first, then handle the restart later.

Copy link
Member

Choose a reason for hiding this comment

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

I think it was looking at the old code. I'd be fine merging it first, then handle the restart later.

Sounds good, thanks!

@artstorm
Copy link

artstorm commented Jan 24, 2023

@bluwy

I also disabled autoprefixer in dev. I'm not sure if this was intentional, but enabling it slows down dev a lot so I changed that in the PR too.

I found another side effect from this PR. There are classes in Tailwind that relies on Autoprefixer to work in all current browsers. One being Background Clip. background-clip only works in Chrome with -webkit- prefix. See CanIUse.

Tailwind styling that works perfectly fine in Astro 1.9 in dev, doesn't work in the current beta if using the lastest Chrome.

Example:
image

It would probably be a good idea to keep autoprefixer enabled in dev, as some Tailwind classes rely on it to work across all browsers. Otherwise it will probably be quite confusing when working in dev if certain parts of Tailwind does not work in popular browsers, like Chrome.

@bluwy
Copy link
Member Author

bluwy commented Jan 24, 2023

I think I saw this issue in Discord too. We can definitely add an option to turn on autoprefixer in dev, but I still think it's good to turn it off by default to have good performance. Usually it's expected that developers work in evergreen browsers, but background-clip seems to be a special case that's not supported by most evergreen browsers.

Another option is to turn on background-clip processing only in dev. Could you help create an issue so we can track this? 🙏

royeden added a commit to royeden/astro that referenced this pull request Jan 27, 2023
…tup) that removed autoprefixer in non build modes
@chenxsan
Copy link
Contributor

chenxsan commented Jan 27, 2023

Usually it's expected that developers work in evergreen browsers

That's not true if your project targets any old browsers, you would need to test them when developing, not after building.

I think Astro can provide an option for users who want to disable autoprefixer in dev env, but please don't turn that option on by default as it could break things like the issue #5989

@williameckert1
Copy link

williameckert1 commented Jan 28, 2023

We can definitely add an option to turn on autoprefixer in dev, but I still think it's good to turn it off by default to have good performance.

Please do, but ideally with Autoprefixer on by default.
The issue with background-clip was confusing me a lot today (my fault for not reading the changelog in detail). But this new default makes it impossible to preview the UI correctly across browsers.

@badalsaibo
Copy link

Hey, I upgraded from v1.0.0 to v2.6.0 and there was an error regarding this config.style.postcss not found? Any work arounds for that? Thanks

@bluwy
Copy link
Member Author

bluwy commented Jul 12, 2023

Make sure to upgrade all the astro-related dependencies together.

@badalsaibo
Copy link

So the astro update itself happens when adding the official netflify adapter. Is there any command to upgrade all astro required dependencies?

@bluwy
Copy link
Member Author

bluwy commented Jul 12, 2023

npm update @astrojs/tailwind @astrojs/other-stuff. Please open a support thread in Discord for further support 😬

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: astro Related to the core `astro` package (scope) pkg: integration Related to any renderer integration (scope) semver: major Change triggers a `major` release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants