Skip to content

Conversation

@lyzadanger
Copy link
Contributor

This PR adds Tailwind support to the CSS build task in sass. It keeps to the convention of adding tailwindcss as a peerDependency, which means that downstream apps that use this package's CSS-building task will be required to install tailwindcss or will crash on build. We can certainly discuss handling this another way.

Running source (S)CSS through tailwindcss is a no-op if the downstream application doesn't provide configuration to the CSS-build task (and also make use of Tailwind directives in its source SCSS).

The Tailwind plugin's internals assumes that the file specified in the
`from` option in the PostCSS config exists and tries to `stat` it. This
failed on first build because we were passing the output CSS path for
this option.

We don't have an input CSS file, because the CSS is generated from SASS
inputs and only exists in memory, but we do have an input .scss file
path, so use that instead.
Copy link
Contributor

@robertknight robertknight left a comment

Choose a reason for hiding this comment

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

I pushed a fix for the build error if the output does not already exist. I also pushed an example of how we could make Tailwind an optional dependency for projects that don't need it. We should chat about whether this is something we want to do or not.

Comment on lines +25 to +26
* @param {TailwindConfig} [options.tailwindConfig]
* Optional tailwind config object
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest to remove this comment line, as it essentially restates the type info.


const postcssResult = await cssProcessor.process(sassResult.css, {
from: output,
from: input,
Copy link
Contributor

Choose a reason for hiding this comment

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

I changed this to fix an error if the output file does not already exist. See notes in commit.

"rollup": "^2.58.0",
"sass": "^1.43.2"
"sass": "^1.43.2",
"tailwindcss": "^2.2.19"
Copy link
Contributor

Choose a reason for hiding this comment

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

It might make sense to make this an optional peer dependency for downstream projects that don't use Tailwind. This is done in the peerDependenciesMeta section - https://classic.yarnpkg.com/en/docs/package-json#toc-peerdependenciesmeta. Additionally you'll need to put any optional functionality under a separate entry point or use dynamic import to load it.

Example of making Tailwind optional:

diff --git a/lib/sass.js b/lib/sass.js
index e07139e..723f098 100644
--- a/lib/sass.js
+++ b/lib/sass.js
@@ -4,7 +4,6 @@ import { basename, dirname, extname } from 'path';
 import autoprefixer from 'autoprefixer';
 import postcss from 'postcss';
 import sass from 'sass';
-import tailwindcss from 'tailwindcss';
 
 /**
  * @typedef {import('tailwindcss/tailwind-config').TailwindConfig} TailwindConfig
@@ -34,6 +33,14 @@ export async function buildCSS(
   const minify = process.env.NODE_ENV === 'production';
   await mkdir(outDir, { recursive: true });
 
+  /** @type {typeof import('tailwindcss')} */
+  let tailwindcss;
+  try {
+    tailwindcss = (await import('tailwindcss')).default;
+  } catch {
+    // Ignored
+  }
+
   await Promise.all(
     inputs.map(async input => {
       const output = `${outDir}/${basename(input, extname(input))}.css`;
@@ -46,10 +53,12 @@ export async function buildCSS(
         sourceMap: sourcemapPath,
       });
 
-      const cssProcessor = postcss([
-        tailwindcss(tailwindConfig),
-        autoprefixer(),
-      ]);
+      const optionalPlugins = [];
+      if (tailwindcss) {
+        optionalPlugins.push(tailwindcss(tailwindConfig));
+      }
+
+      const cssProcessor = postcss([...optionalPlugins, autoprefixer()]);
 
       const postcssResult = await cssProcessor.process(sassResult.css, {
         from: input,
diff --git a/package.json b/package.json
index ec7ef8a..c82f751 100644
--- a/package.json
+++ b/package.json
@@ -42,6 +42,11 @@
     "sass": "^1.43.2",
     "tailwindcss": "^2.2.19"
   },
+  "peerDependenciesMeta": {
+    "tailwindcss": {
+      "optional": true
+    }
+  },
   "prettier": {
     "arrowParens": "avoid",
     "singleQuote": true

@lyzadanger
Copy link
Contributor Author

Thanks, @robertknight ! I've taken your suggestion of making Tailwind an optional dependency and then tested this against master of the client, as well as hypothesis/client#3958

It may end up being the case that Tailwind will be a hard dependency for applications using frontend-shared if frontend-shared starts using Tailwind directives in its provided SASS modules. Ultimately it would be cool if frontend-shared provided build CSS instead of SASS, but until it does, the consuming applications need to be able to build that SASS, and that SASS is going to have some Tailwind in it soon. We can keep talking about this. I still think a nod toward Tailwind being optional is a nice gesture.

We good to land and release here?

@lyzadanger lyzadanger merged commit e7e5b3d into main Dec 2, 2021
@esanzgar esanzgar deleted the tailwind-support branch January 10, 2022 16:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants