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

postcss import resolution not working when css loader set to "text" (monorepo only) #770

Closed
rondonjon opened this issue Nov 11, 2022 · 9 comments · Fixed by #773
Closed
Labels

Comments

@rondonjon
Copy link

rondonjon commented Nov 11, 2022

#743 has requested support CSS inlining as an alternative to generating additional output files.

The suggested solution was to set the .css loader to text, which wasn't working initially but has been fixed in tsup 6.3.0.

While the contents are now "inlined", it seems that they are no longer preprocessed by postcss, as one would expect from "CSS inlining".

With the default settings, tsup generates an external CSS of more than 150 KB in size ...

IIFE ../web/public/app/index.css 150.75 KB

... but after setting the .css loader to text, I am now getting a very short (inlined) CSS snippet which still contains imports that postcss should have resolved:

@import './themes/a.css';
@import './themes/b.css';
@import './themes/c.css';

Actually I find this behavior quite logical and consistent and would propose to leave it just how it is. If the content were preprocessed by postcss, the term (plain) "text" would be misleading. But that obviously doesn't solve the original request.

Could we have a new loader option inline-css to make this more explicit and meet the original request?

Or, more generically, would it make sense to split these configs into at least two parameters, "loading" (inlined vs. extra file) and "transformation" (e.g. typescript, svelte, css)?

@await-ovo
Copy link
Contributor

await-ovo commented Nov 14, 2022

If you want to resolve paths for @import rules when switching .css loader to text,
I think just adding postcss-import plugin to your postcss.config.jswill solve your problem.

@rondonjon
Copy link
Author

rondonjon commented Nov 14, 2022

There is already a postcss.config.js with the following contents:

plugins: [
    'postcss-import',
    'tailwindcss/nesting',
    'autoprefixer',
    'tailwindcss',
  ],

This config is working fine when with the "css" loader and will then produce a big standalone CSS file which has all CSS imports resolved. With the "text" loader however the config seems to be ignored.

@await-ovo
Copy link
Contributor

await-ovo commented Nov 14, 2022

Hi @rondonjon, I am interested in this problem, could you please provide a repository to reproduce this?

image

In my case, it seems that postcss works fine with the text loader, here is dist/index.js for above example:

image

@rondonjon
Copy link
Author

rondonjon commented Nov 14, 2022

Thanks @await-ovo!

Here you go: https://gitlab.com/rondonjon/tsup-postcss-reproduction

While setting this up I noticed that the problem only occurs in a yarn monorepo. But then it is fully reproducible:

Test base

  • Package @repro/app with an index.ts and a tsup build configured through tsup.config.mjs
  • Package @repro/styles with an a.css that imports a b.css

Style output with tsup and "css" loader

Expected result: a.css and b.css are merged, minified, and written to index.css
Actual result: 🟢

body{padding:0;margin:0}html{background:red}

Style output with tsup and "text" loader

Expected result: a.css and b.css are merged. minified, and inlined in index.js
Actual result: 🔴 import of b.css is not resolved, CSS is not minified -> is postcss executed at all ???

"use strict";(()=>{var r=`@import "./b.css";

html {
  background: red;
}
`;alert(r);})();

@rondonjon rondonjon changed the title postcss transformation not working when css loader set to "text" postcss transformation not working when css loader set to "text" (monorepo only) Nov 14, 2022
@rondonjon rondonjon changed the title postcss transformation not working when css loader set to "text" (monorepo only) postcss import resolution not working when css loader set to "text" (monorepo only) Nov 14, 2022
@await-ovo
Copy link
Contributor

await-ovo commented Nov 14, 2022

Currently tsup search postcss.config.js based on the path the css file, and if it doesn't find it, it skips the postcss transform, because there is no postcss.config.js in packages/styles directory, so @app/styles/a.css will not be processed by postcss.

The temporary solution is add a postcss.config.js to the packages/styles directory so that @import rule can be transformed.

I think tsup should search for postcss.config.js in the root of the project directory like vite does

@rondonjon
Copy link
Author

@await-ovo, thanks again, this is really helpful, and working around the issue is good enough for me at the moment. Out of curiosity, could you explain why this only affects the "text" loader but not the "css" loader?

@await-ovo
Copy link
Contributor

await-ovo commented Nov 14, 2022

@await-ovo, thanks again, this is really helpful, and working around the issue is good enough for me at the moment. Out of curiosity, could you explain why this only affects the "text" loader but not the "css" loader?

i think esbuild itself will resolve @import in css files with css loader~

@rondonjon
Copy link
Author

What is your take on this @egoist ?

tsup is resolving the location of the postcss.config.js file differently in the "text" and the "css" loader when the imported css file is outside the tsup directory (here: other package in same monorepo).

It seems arguable which resolution makes more sense (based on the location of the source file or on the tsup/project root), personally I tend towards the tsup/project root, but I think the resolution should be consistent across all loaders.

@github-actions
Copy link

github-actions bot commented Feb 7, 2023

🎉 This issue has been resolved in version 6.6.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

SeanZellmer added a commit to SeanZellmer/tsup that referenced this issue Feb 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants