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

Update Plugin to support Tailwind Intellisense #650

Closed
endigo9740 opened this issue Dec 7, 2022 · 14 comments · Fixed by #766
Closed

Update Plugin to support Tailwind Intellisense #650

endigo9740 opened this issue Dec 7, 2022 · 14 comments · Fixed by #766
Labels
enhancement New feature or request ready to test Ready to be tested for quality assurance.
Milestone

Comments

@endigo9740
Copy link
Contributor

Current Behavior

I've heard reports it works for Daisy UI, and am sure it works locally in the Skeleton project, so we'll need to see how to carry this through to end users.

Steps To Reproduce

No response

Anything else?

No response

@endigo9740 endigo9740 added bug Something isn't working enhancement New feature or request help wanted Extra attention is needed labels Dec 7, 2022
@endigo9740 endigo9740 added this to the v1.0 milestone Dec 7, 2022
@endigo9740
Copy link
Contributor Author

endigo9740 commented Dec 8, 2022

Note that @AdrianGonz97 (@CokaKoala on Discord) has made some headway on this. I've asked him to share hit notes when he gets a chance.

@AdrianGonz97
Copy link
Member

AdrianGonz97 commented Dec 10, 2022

I haven't had too much time to further look into it in the past couple of days besides getting some intellisense to work, but after playing around with tailwind's plugin system now, I think I have a decent idea for what we'd have to do to get this working.

The gist of it is that we have to transpile all of our CSS into CSS-in-JS to then be able to add our classes as a tailwind plugin. Without adding them as a plugin, we won't receive any intellisense for them. This is the reason why we get intellisense for classes such as bg-surface-900 but not for btn-ghost.

Since we split our stylesheets into different purposes, we have a few approaches to go with:

Importing ALL

  • We first have to combine all of our different CSS layers into their respective groups (ex. @layer components, @layer base, @layer utilities, etc...)
  • Transpile each group of stylesheets with postcss into vanilla CSS
  • Convert the vanilla CSS files into CSS-in-JS syntax as a commonjs exported object (using prejss-cli or postcss-js)
  • Add those CSS-in-JS objects into the tailwind plugin via tailwind's helper functions addComponents addBase addUtilities

After this, all we'd need the users to do is to import the all.cjs plugin in their tailwind.config.cjs file as so:

// ...
    plugins: [
        require('@skeletonlabs/skeleton/tailwind/theme.cjs'),
        require('@skeletonlabs/skeleton/tailwind/all.cjs')
    ]

No need for importing these stylesheets into the root layout file anymore.

Selective imports

Currently, we have the option for users to selectively choose their stylesheets. To get this working, we could make each available stylesheet into their own plugin to maintain this behavior. For example, rather than doing this in the root layout:

import '@skeletonlabs/skeleton/styles/{stylehsheets}.css';	

We could instead have them imported as plugins in their tailwind.config.cjs file as so:

// ...
    plugins: [
        require('@skeletonlabs/skeleton/tailwind/theme.cjs'),
        require('@skeletonlabs/skeleton/tailwind/tokens.cjs'),
        require('@skeletonlabs/skeleton/tailwind/core.cjs'),
        // etc...
    ]

The steps to convert these stylesheets into their own plugins will be similar to how we did it above with the all.cjs plugin, but without the first step of grouping.

Advantages

If it works how I think it works, then we can gain some features doing it this way:

  • Intellisense (woohoo)
  • All of the unused styles will now be stripped out by tailwind, where currently, if we imported the all.css stylesheet, everything gets bundled into our app - even the classes that are unused.
  • It would be easier to use(?) since we don't have to juggle around with the @tailwind directives anymore. The user can just have them in the app.postcss and be done with it.

Side Note:

I still don't know if this would unexpectedly break things. I'm going to experiment with it more today and find out.

At the moment, I have to do everything manually. I'm sure there's a script I can come up with to do it all at build time, but for the sake of prototyping, this is how I'll painfully do it 😆.

If there's anything that I've missed, please let me know! If there's a better way to accomplish this, then I've yet find it. And finally, I welcome all ideas to better this process!

@endigo9740
Copy link
Contributor Author

@AdrianGonz97 So I went down a similar rabbit hole a few weeks ago. I'm aware that Tailwind wants everything in CSS-in-JS format, which personally speaking, is a huge mistake. It's incredibly cumbersome to write and doesn't provide the speed or flexibility of writing either vanilla CSS or using @apply. It's hard to find documentation on how to replicate trivial things like color variable references.

You're right, this does explain why we're seeing our color values but nothing else.That said, if we can find a way to automatically transpile then that would be great, but I was never able to find tooling for this. If you've come across something like this please definitely do share. We definitely won't be rewriting or converting this manually. The last time I tried that it took near a whole afternoon to translate HALF of typography.css, which is a very basic sheet. With our recent addition of the CSS tokens we have greatly increased the number of styles being provided, so this will need to be handled in an automated fashion to be viable.

The other point that sticks out above is when you say "All of the unused styles will now be stripped out by tailwind". Both @niktek and I were under the impression PostCSS and/or Vite was doing tree shaking for us. If that's not the case that needs to be an immediate priority. I understand Tailwind could handle this in a more efficient manner, but again, with the addition a slew of design token classes we're going to be really inflating folk's build size if everything is being included. It's CSS, so it's not that heavy or blocking like JS, but still, we should aim for efficiency here. After the upcoming release Tuesday I'm going to move this up to top priority.

One thing to keep in mind is that Tailwind 4 is on the horizon. If you're on Twitter I'd recommend you read through Adam Wathan (creator of Tailwind)'s account:

He has been sharing thoughts and concepts for v4 here and there. One of the details that sticks out to me is this idea of moving more of the config to from JS -> CSS. That initially seemed odd to me, but perhaps it's a sign they are reducing their dependence on JS-in-CSS? I guess we can only hope.

There's no ETA for this, so we definitely need some form of tree shaking in the short term to reduce bloat. I'm open to any an all ideas on this though.

@AdrianGonz97
Copy link
Member

It's incredibly cumbersome to write and doesn't provide the speed or flexibility of writing either vanilla CSS or using @apply. It's hard to find documentation on how to replicate trivial things like color variable references.

I have definitely felt the pain of this yesterday. The documentation around it is very lacking and hunting down tooling for it has been a nightmare.

That said, if we can find a way to automatically transpile then that would be great, but I was never able to find tooling for this. If you've come across something like this please definitely do share.

Currently, I've been successful in transpiling the all.css file into a large glob of CSS-in-JS. I then took that and added it as a tailwind plugin. That's how I was able to successfully get intellisense working that first time. I still don't know the possible side-effects of it, but I'll open a PR soon so you can try it out and see if you'd might be able to spot inconsistencies.

The other point that sticks out above is when you say "All of the unused styles will now be stripped out by tailwind". Both @niktek and I were under the impression PostCSS and/or Vite was doing tree shaking for us. If that's not the case that needs to be an immediate priority.

I could be wrong about this, but from what I've gathered when I built my project and examined it yesterday, I noticed that all of the styles, even unused ones, were present in the build files. Perhaps vite does additional processing at runtime? I would have to investigate it further.

He has been sharing thoughts and concepts for v4 here and there. One of the details that sticks out to me is this idea of moving more of the config to from JS -> CSS. That initially seemed odd to me, but perhaps it's a sign they are reducing their dependence on JS-in-CSS? I guess we can only hope.

That would certainly be a blessing.


So far, trying to get this to work has been a real pain in my side 😆. I've written some scripts in the package.json to transpile the files for us, but getting it to work seamlessly in dev has been a real challenge. At the moment, if you were to change the contents of any of the stylesheets, you'd have to manually rerun the package script. I've been trying to think of
a good way to automate that process through some directory watcher.

I'll make a PR so you can track my progress and to try out.

@endigo9740
Copy link
Contributor Author

Ok @AdrianGonz97 thank you for the detailed response and looking into this. I'm juggling a lot this week, between illness, release, and interview on Thursday, but let me know of myself or @niktek can help. Once we can get everything else out of the way this will be the priority for sure, or at least testing tree shaking. Otherwise we might have some upset folks after this upcoming release when their CSS bundle like doubles in size!

@endigo9740 endigo9740 changed the title VS Code Tailwind extension doesn't provide Intellisense for Skeleton styles Tailwind Intellisense and CSS Tree Shaking Dec 12, 2022
@endigo9740 endigo9740 pinned this issue Dec 12, 2022
@endigo9740
Copy link
Contributor Author

endigo9740 commented Dec 13, 2022

@AdrianGonz97 @niktek Here's a summary of today's findings:

  • Neither Daisy or Flowbite use direct stylesheet imports. Instead, everything appears to go through their respective Tailwind plugin
  • This plugin has a generic name like flowbite/plugin or daisyui, so we should probably follow this lead
  • By using the Tailwind Plugin we should get Intellsense and a smaller initial bundle size
  • However, given that the Tailwind config must be pointed at the node_modules/package, it may pickup all component styles by default
  • The help combat this, we should follow Flowbite's lead and recommend the use of a post-process cleanup method using a tool like Purge CSS. Which has a PostCSS plugin that's (hopefully) trivial to setup.
  • Daisy currently uses prejss-cli while Flowbite uses postcss-cli. Not sure how specifically they are handling transpiling yet though.

However, the problem then becomes how we get our stylesheets transpiled and merged into the plugin. For this we'll probably need to lean into a tools like prejss-cli or postcss-js. However we have several complication to overcome here:

  1. We need to be able to see a live preview while we're making changes. So perhaps in the dev environment we still use the raw stylesheets. But for end users they use the "pre-baked" plugin generated on package?
  2. Not all stylesheets live within the NPM package directory. Very specifically the theme file needs to be set and controlled by developers.
  3. Also the theme needs to be the FIRST set of styles imported. Otherwise all other styles that depend on these CSS properties won't operate and will most likely error. Perhaps we keep that as a manual import?
  4. Finally we need to confirm the format for the stylesheets. We want to ensure they continue to be easy to override locally in the project, and can still be use as utility classes, including the use of variants like media breakpoints md: and dark mode dark:

Given the scale and scope of the plugin changes, I'd say we start by implementing the Purge CSS suggestions first and foremost. Then move our attention to the Tailwind plugin.

Thoughts?

@endigo9740
Copy link
Contributor Author

Additionally I reached out to Zoltan from Flowbite. He said they put they manually add and maintain additional styles via the CSS-in-JS for their plugin, however they have a very limited number of these. Opting to avoid opinionated classes wherever possible.

We on the other hand depend heavily on the use of a large number of stylesheets and classes, primarily due to our element and design token classes. So really I expect one of two things for us:

  1. We find a sane way to transpile and migrate everything over to that
  2. We don't find a way to handle this and have to continue to rely on the current CSS import methods

I'll plan to do more research into the transpile options tomorrow as time allows.

@endigo9740 endigo9740 changed the title Tailwind Intellisense and CSS Tree Shaking Update Plugin to support Tailwind Intellisense Dec 19, 2022
@endigo9740
Copy link
Contributor Author

FYI I'm splitting off the tree shaking portion of this via Purge CSS to it's own ticket here:

This current ticket should remain focused on the CSS-in-JS transpiling solutions for our Tailwind plugin. Assuming this is feasible.

@endigo9740 endigo9740 unpinned this issue Dec 22, 2022
@endigo9740
Copy link
Contributor Author

endigo9740 commented Dec 27, 2022

@AdrianGonz97 @niktek I've come up with an idea that might get us -part- of the way there on this issue. Allowing us to implement our design tokens as part of the TW plugin.

If you look at what our design tokens are, there is a TON of really small but repetitive classes with only minor changes.
https://github.com/skeletonlabs/skeleton/blob/dev/src/lib/styles/tokens/backgrounds.css

Here's one example:

/* Primary */
.bg-primary-50-900-token {
	@apply bg-primary-50 dark:bg-primary-900;
}
.bg-primary-100-800-token {
	@apply bg-primary-100 dark:bg-primary-800;
}
.bg-primary-200-700-token {
	@apply bg-primary-200 dark:bg-primary-700;
}
.bg-primary-300-600-token {
	@apply bg-primary-300 dark:bg-primary-600;
}
.bg-primary-400-500-token {
	@apply bg-primary-400 dark:bg-primary-500;
}

Here we're creating a set of base/dark mode stepped color values for the Primary color. Then we immediately go through and repeat this for secondary, tertiary, etc. If we were doing this with Javascript we would just use a loop - right? Well with the Tailwind plugin we CAN.

Here's some quick pseudo code. This is untested but should explain the process:

export const colorNames = ['primary', 'secondary', 'tertiary', 'success', 'warning', 'error', 'surface'];

function createColorPairingTokens() {
	colorNames.forEach(n => {
		return {
			[`bg-${n}-50-900-token`]: `bg-${n}-50 dark:bg-${n}-900`,
			[`bg-${n}-100-800-token`]: `bg-${n}-100 dark:bg-${n}-800`,
			// ...
		};
	});
}

NOTE: I've used tailwind syntax here for the styling, but this would need to be translated to CSS-in-JS to work properly

This will totally work in our plugin, and is very similar to how we're integrating our color values currently:
https://github.com/skeletonlabs/skeleton/blob/dev/src/lib/tailwind/theme.cjs

I think we could generate a LARGE portion of our tokens classes like this, which account for a large portion of the bundle size. This would also provide Intellisense for these classes.

Unfortunately components aren't subject to the same conditions that we could easily loop and replicate them. However, as far as bundle size and Intellisense, this is a big step forward.

Thoughts?

@ryceg
Copy link
Contributor

ryceg commented Dec 27, 2022

Huge major thumbs up from me. Much cleaner. Can't think of any downsides.

@endigo9740 endigo9740 pinned this issue Jan 3, 2023
@endigo9740
Copy link
Contributor Author

endigo9740 commented Jan 3, 2023

@AdrianGonz97 sharing my progress here, plus one issue that perhaps you can help nail down. In short it's working perfectly for light mode, but dark mode's syntax is giving me trouble.

I've modified theme.cjs as follows:

// Skeleton Tailwind Plugin - Theme Colors
// - Extends color palette to accept themeable CSS variables

const plugin = require('tailwindcss/plugin');

// Source: https://tailwindcss.com/docs/customizing-colors#using-css-variables

function createColorSet(colorName) {
	return {
		50: `rgb(var(--color-${colorName}-50) / <alpha-value>)`,
		100: `rgb(var(--color-${colorName}-100) / <alpha-value>)`,
		200: `rgb(var(--color-${colorName}-200) / <alpha-value>)`,
		300: `rgb(var(--color-${colorName}-300) / <alpha-value>)`,
		400: `rgb(var(--color-${colorName}-400) / <alpha-value>)`,
		500: `rgb(var(--color-${colorName}-500) / <alpha-value>)`,
		600: `rgb(var(--color-${colorName}-600) / <alpha-value>)`,
		700: `rgb(var(--color-${colorName}-700) / <alpha-value>)`,
		800: `rgb(var(--color-${colorName}-800) / <alpha-value>)`,
		900: `rgb(var(--color-${colorName}-900) / <alpha-value>)`
	};
}

const colorNames = ['primary', 'secondary', 'tertiary', 'success', 'warning', 'error', 'surface'];
// const colorShades = [50, 100, 200, 300, 400, 500, 600, 700, 800, 900];
const colorPairings = [
	// forward:
	{ light: 50, dark: 900 },
	{ light: 100, dark: 800 },
	{ light: 200, dark: 700 },
	{ light: 300, dark: 600 },
	{ light: 400, dark: 500 },
	// backwards
	{ light: 900, dark: 50 },
	{ light: 800, dark: 100 },
	{ light: 700, dark: 200 },
	{ light: 600, dark: 300 },
	{ light: 500, dark: 400 },
];

function createTokensBackground() {
	const tokensObj = {};

	colorNames.forEach(n => {
		colorPairings.forEach(p => {
			tokensObj[`.bg-${n}-${p.light}-${p.dark}-token`] = { 'background-color': `rgb(var(--color-${n}-${p.light}))` };
			tokensObj[`.dark .dark:bg-${n}-${p.light}-${p.dark}-token`] = { 'background-color': `rgb(var(--color-${n}-${p.dark}))` };
		});
	});

	console.log(tokensObj);

	return tokensObj;
}

module.exports = plugin(({ addUtilities }) => {
	addUtilities({
		...createTokensBackground()
	})
}, {
	theme: {
		extend: {
			// Extend the colors with the CSS variable values
			// NOTE: Must be RGB to allow for TW opacity value
			colors: {
				primary: createColorSet('primary'),
				secondary: createColorSet('secondary'),
				tertiary: createColorSet('tertiary'),
				success: createColorSet('success'),
				warning: createColorSet('warning'),
				error: createColorSet('error'),
				surface: createColorSet('surface')
			}
		}
	}
});

The result is it generates a whole slew of classes. Something like 70 unique classes, with a light and dark variation per each. So technically 140 classes. This equates to around 300 lines of CSS removed from backgrounds.css.

Screen Shot 2023-01-03 at 3 28 01 PM

We're also seeing Intellisense working. Note all the token variations here:

Screen Shot 2023-01-03 at 3 01 13 PM

The results in light mode are perfect. This is with the CSS tokens removed, and relying on the generated classes in the plugin:

Screen Shot 2023-01-03 at 2 53 31 PM

However, dark mode is busted. The correct class/scope is not being set:

Screen Shot 2023-01-03 at 2 53 38 PM

I believe this to be related to the odd syntax Tailwind uses, note the \: bit in this example:

Screen Shot 2023-01-03 at 2 51 00 PM

It seems .cjs files or the bundler do not like the use of backslashes within strings. Normal escape conventions are not working:

  • .dark .dark\:bg- generates .dark .dark:bg- (missing the slash)
  • .dark .dark\\:bg- generates .dark .dark\\:bg- (double slash)

There does not appear to be a manner for me to implement a single backsplash within these string, thus things are busted. If we can figure out a solution for this though I think we're golden. I can start converting more tokens over to this system!

@endigo9740
Copy link
Contributor Author

User @gabbydd on Discord pointed out that it's very easy to miss the dot before bg in .dark .bg-. Adding this resolves the issue and enables dark mode to work as well! So I'm going to begin the process of migrating tokens over to this format now. This may carry through to tomorrow or so, but hopefully it'll go fast once I get rolling!

@niktek @AdrianGonz97 FYI I do plan to modify the name of our plugin to something more generic like Daisy and Flowbite do, so skeleton.cjs or similar. But will handle all dynamic generation of these classes going forward and replace the CSS imports permanently.

@niktek I am pinging you as you may need to modify the CLI to use the new plugin name convention. Please keep in the loop as we progress towards this for the new release next week.

@endigo9740 endigo9740 linked a pull request Jan 4, 2023 that will close this issue
@endigo9740 endigo9740 added ready to test Ready to be tested for quality assurance. and removed help wanted Extra attention is needed bug Something isn't working labels Jan 5, 2023
@endigo9740
Copy link
Contributor Author

With that I think the new plugin PR is ready to test:

Obviously you can look at the doc site in the preview, but the best test will be ensuring this works as expect in either a new fresh Skeleton project or an existing one. For this I'm going to attach a package tarball:
skeleton-test-package.tgz

You can follow the instructions here to install this:
https://github.com/skeletonlabs/skeleton/wiki/Create-and-Install-Test-Packages

Obviously be sure to revert to the remote production package when you complete your tests. The things to look out for include:

  • You'll need to migrate to the new Tailwind plugin name, from theme.cjs to skeleton.cjs in your tailwind.config.cjs file
  • Ensure the basics are working, no visible terminal or console errors
  • Ensure that most styles are working as expected. This would include theme colors and default component styles if it's failing.

It'll be visually obvious if it's not working. I'm going to run through and test this in a new CLI project, but I welcome help from anyone that can provide it given this is a critical part of how Skeleton operates. Thanks!

@endigo9740
Copy link
Contributor Author

FYI I've run a quick test using a newly generated app via the CLI. All is working as expected. Note that you may see a warning in relation to a theme import for fonts. This is unrelated and a bug ticket has been filed:

@endigo9740 endigo9740 unpinned this issue Jan 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request ready to test Ready to be tested for quality assurance.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants