tailwindlabs / tailwindcss-intellisense

Intelligent Tailwind CSS tooling for Visual Studio Code
2.86k stars 195 forks source link

Bug report: Plugin-added colors not displaying in intellisense pop-up/context window #544

Closed brandonmcconnell closed 2 years ago

brandonmcconnell commented 2 years ago

What version of Tailwind CSS IntelliSense are you using?

v0.8.3

What version of Tailwind CSS are you using?

v3.0.24

What package manager are you using?

npm and rush (same result with both)

What operating system are you using?

macOS Monterey v12.3.1 (21E258)

Reproduction steps

  1. Install tailwind-lerp-colors into a project using npm install tailwind-lerp-colors. (related repo)
  2. Initialize the plugin first before other plugins in the Tailwind config using require('tailwind-lerp-colors') file (using defaults w/o invoking should be fine to reproduce the error).
  3. (optionally) Run build script. I did this just in case it was a factor for Tailwind CSS Intellisense.
  4. Start typing the name of one of the newly interpolated colors' class-names (e.g. bg-red-350) and you should see that the newly added interpolated colors do not show up in the intellisense pop-up/context window. However, if you save, these new colors do work as all other colors do and can be used with Tailwind JIT.

Describe your issue

I built a Tailwind plugin that interpolates between colors from the Tailwind config using the interval and color-space of the user's choice (among other user-definable options). The plugin works, and these new colors do work as all other colors do and can be used with Tailwind JIT, but the new colors do not display in the Tailwind CSS Intellisense pop-up/context window.

ripvannwinkler commented 2 years ago

Can confirm the same issue with a custom buttons plugin I built:

/* eslint-disable @typescript-eslint/no-var-requires */
const plugin = require('tailwindcss/plugin');
const color = require('tinycolor2');

module.exports = plugin(function ({ addComponents, theme }) {
  console.log(mapColors(theme('colors')));
  addComponents({
    '.btn': {
      display: 'block',
      padding: '.5rem 1.35rem',
      borderRadius: '.2rem',
      fontWeight: '500',
    },
    '.btn-small': {
      padding: '.4rem 1rem',
      fontSize: '0.65rem',
    },
    ...mapColors(theme('colors')),
  });
});

function mapColors(colors, prefix = '', result = {}) {
  Object.keys(colors).map((key) => {
    const value = colors[key];
    key = key.toLowerCase();
    key = key === 'default' ? '' : `-${key}`;
    if (typeof value === 'string') {
      makeSolidVariant(result, prefix, key, value);
      makeOutlineVariant(result, prefix, key, value);
    } else if (typeof value === 'object') {
      mapColors(value, key, result);
    }
  });

  return result;
}

function makeSolidVariant(result, prefix, key, value) {
  // normal state
  result[`.btn${prefix}${key}`] = {
    transition: 'background-color 0.15s linear',
    backgroundColor: value,
    color: color(value).isDark() ? '#ffffff' : '#000000',
  };
  [
    // interactive states
    `.btn${prefix}${key}:not(:disabled):hover`,
    `.btn${prefix}${key}:not(:disabled):focus`,
    `.btn${prefix}${key}:not(:disabled):active`,
  ].map(
    (c) =>
      (result[c] = {
        backgroundColor: color(value).darken(10).toRgbString(),
        color: color(value).isDark() ? '#ffffff' : '#000000',
      }),
  );
}

function makeOutlineVariant(result, prefix, key, value) {
  // normal state
  result[`.btn-outline${prefix}${key}`] = {
    transition: 'background-color 0.15s linear',
    backgroundColor: '#ffffff',
    border: `1px solid ${value}`,
    color: value,
  };
  [
    // interactive states
    `.btn-outline${prefix}${key}:not(:disabled):hover`,
    `.btn-outline${prefix}${key}:not(:disabled):focus`,
    `.btn-outline${prefix}${key}:not(:disabled):active`,
  ].map(
    (c) =>
      (result[c] = {
        backgroundColor: value,
        border: `1px solid ${value}`,
        color: color(value).isDark() ? '#ffffff' : '#000000',
      }),
  );
}

Notice in this screenshot, the built in text-primary decorator shows up, but no decorator for the the usage of btn-primary:

image

The color does show up when rendered, so it's just the decorator that's not picking up the additional styles from the plugin.

brandonmcconnell commented 2 years ago

@ripvannwinkler Having some way to preview the styles for a custom class would be cool too, though I think that may deserve its own separate feature request, as custom classes do not currently support color previews (to my knowledge).

When color-specific classes like bg-aqua-400 are generated using colors from the Tailwind config colors:

{
  theme: {
    extend: {
      colors: {
        aqua: {
           50: '#eefdf8',
          100: '#cffbeb',
          200: '#a0f5da',
          300: '#66e9c6',
          400: '#31d4ad',
          500: '#12b995',
          600: '#0a9579',
          700: '#0b7763',
          800: '#0d5f50',
          900: '#0e4e43',
        }
      }
    }
  }
}

When these colors are added manually to theme.colors or theme.extend.colors, Tailwind CSS Intellisense accounts for them and displays the color previews, but when they are added programmatically via a plugin, it does not.

This is essentially what my plugin, tailwind-lerp-colors does. It adds new colors to the Tailwind config at theme.extend.colors. The classes are generated and this does work (i.e. Tailwind JIT does support the programmatically generated colors/classes, but Tailwind CSS Intellisense does not show the class names or the color previews.

The difference I see in your example is that your classes are not utilities but rather composite classes that combine a number of styles, so they're not actual Tailwind color-specific colors, so Intellisense doesn't by nature show any color previews for them BUT your classes should still appear in the intellisense pop-up window as an available class-name to select.

ripvannwinkler commented 2 years ago

@brandonmcconnell yes, the classes still show up in intellisense and they do work. Thanks.

adamwathan commented 2 years ago

I can't reproduce this using a simple custom plugin that adds colors to the config:

// tailwind.config.js
const plugin = require("tailwindcss/plugin");

module.exports = {
  content: [],
  theme: {
    extend: {},
  },
  plugins: [
    plugin(function () {}, {
      theme: {
        extend: {
          colors: {
            flamingo: "#eb34c3",
          },
        },
      },
    }),
  ],
};

image

image

My gut is the source of your problem is specific to your implementation, and the fact that you are relying on mutating module level variables inside your plugin function. Tailwind runs your configuration extensions first and that's what I'm guessing IntelliSense is seeing, and at the time that runs, finalColors is still an empty object.

I think what you're really after here is being able to access the current theme inside your own theme configuration, which you should be able to do if I'm not mistaken by using the function syntax:

const plugin = require('tailwindcss/plugin')

module.exports = {
  content: [],
  theme: {
    extend: {},
  },
  plugins: [
    plugin(() => {}, {
      theme: {
        extend: {
          colors: ({ theme }) => {
            return somethingThatUsesTheme;
          }
        }
      }
    })
  ],
}

I'd try refactoring to use that approach, the way your plugin is authored right now isn't really how we intended for the plugin API to be used.

brandonmcconnell commented 2 years ago

@adamwathan Thanks for the insightful response! I really appreciate it. That makes a ton of sense.

I can certainly refactor my code to include all my logic within that final function.

Would that still support the user-defined options object using plugin.withOptions?

adamwathan commented 2 years ago

It should yep!

brandonmcconnell commented 2 years ago

@adamwathan Okay, that got me about halfway there! That refactoring did cause the Intellisense to start working, but pulling from theme.colors only appears to pull colors defined within the main theme object, and attempting to reference theme.extend.colors throws an undefined error.

Is there a way in which, using the theme syntax you recommended, I can also reference any colors defined within theme.extend.colors? I know many Tailwind users add their colors there, so I would like for my plugin to be able to account for them as well.

brandonmcconnell commented 2 years ago

I've refactored my code to use your suggestion, @adamwathan, but it appears if I reference theme('colors') inside the theme.colors block inside the second plugin.withOptions argument (or even a plain plugin instance argument, without using withOptions), I get a Maximum call stack size exceeded error.

theme.colors (instead of theme('colors')) appears to cause different issues, but that doesn't expose custom colors declared in the theme.extend.colors block, and trying to use theme.extend.colors directly doesn't work.

Version 1.1.0 of my plugin, before making your suggested change, does actually work with Tailwind. It just doesn't play nicely with the Tailwind CSS Intellisense plugin. The change—when using theme.colors—can be made to expose custom colors but defined in the top theme.colors scope (not extend), but this also has its limitations, and in some cases, as the one in this issue opened on my plugin (https://github.com/brandonmcconnell/tailwind-lerp-colors/issues/1), it still causes errors.

Can you please inspect my source to see what else might need to be done to work around the Maximum call stack size exceeded error? I do prefer the method you recommended, especially as it appears to be the way Tailwind CSS Intellisense expects the colors. I just need to resolve that error for the plugin to work properly.

The source can be found here: https://github.com/brandonmcconnell/tailwind-lerp-colors/blob/main/index.js

adamwathan commented 2 years ago

I don't think you're doing anything wrong honestly, we probably just need to do some work to make what you want to do properly possible. Ideally you want to be just reading theme('colors') and have it contain the fully resolved set of colors so you don't have to look at extend at all.

The more I understand what you're trying to do the more I'm convinced the plugin system just doesn't really support that use case well if at all. Not your fault, valid thing to want to do for sure but it's just not going to really work, and I can't say I can justify investing a ton of resources into it or anything for you since we have our own priorities.

I think the best way to make it truly work would be to get users to pass the colors in to your plugin through the options, so you have direct access to the colors they want to use at the right phase in your plugin's lifecycle.

// tailwind.config.js
module.exports = {
  theme: {
    // Don't define any colors here
  },
  plugins: [
    require('tailwind-lerp-colors')({
      // Pass them here instead
      colors: ...
    }),
  ],
}

Or instead of even writing a plugin just give people a tool for generating the colors that returns a color object:

// tailwind.config.js
const lerp = require('tailwindcss-lerp-colors')
const colors = require('tailwindcss/colors')

module.exports = {
  theme: {
    colors: lerp(colors)
  },
  plugins: [],
}
brandonmcconnell commented 2 years ago

Thanks for looking into that some more, @adamwathan. I did consider using tailwind-lerp-colors as a function that wraps colors, whether all colors or individual colors, and there may even be some side-effect benefits to doing it that way.

Going back to my original v1.1.0 implementation, the plugin actually fully worked in Tailwind, and all colors were interpolated as expected. The only issue was that Tailwind CSS Intellisense. I'm wondering if there might be some small change possible that would simply expose all classes Tailwind CSS generates to the Intellisense plugin, even if they are not generated in the way traditionally expected.

In my mind, if Tailwind CSS accepts it, shouldn't Tailwind CSS Intellisense?

Thanks again!

adamwathan commented 2 years ago

Yeah Tailwind accepts it but just by chance, not by design. To me the way it's implemented currently is the sort of thing we could unintentionally break in the future without considering it a breaking change. So I don't plan to invest any time into it unfortunately — maybe it's something @bradlc thinks actually should work and it's just some tiny change he has to make then sure, but otherwise I think it's just one of those "sorry that it doesn't work but we didn't intend for it to work and not interested in working on it" situations. Sorry man 🙁

brandonmcconnell commented 2 years ago

@adamwathan No worries. I can certainly understand that. I'll keep using my accidentally functional v1.1.0 for now, and then if the functional notation is patched to work without causing those Maximum call stack size exceeded errors down the road, I'll make the change to using that instead. Thanks again!

brandonmcconnell commented 2 years ago

@adamwathan The main issue here appears to be the Maximum call stack size exceeded, likely due to an infinite loop trying to reference colors from within a plugin that also writes to colors. Is there a way to prevent a plugin's change from triggering an update on itself to more simply avoid the infinite nature of the loop altogether?

Or somehow spread the colors returned into a separate object in such a way that the Tailwind config does not see it as a change needing to trigger a global update to itself.

I think the first idea above would make the most sense, to prevent any plugins from triggering updates to themselves, to avoid infinite loops.

bradlc commented 2 years ago

Hey @brandonmcconnell. I'm happy to take a look at your original version (v1.1.0) and see if there's an easy way to get IntelliSense working. I'm currently having trouble getting the plugin to work at all though. Would you mind taking a look at my basic repo and seeing if you can see what I'm doing wrong? https://github.com/bradlc/tailwind-lerp-test

brandonmcconnell commented 2 years ago

Hey @bradlc, thanks for looking into this for me! I really appreciate it!

I came across a more significant issue with my current approach this week which would actually require a Tailwind change. If I use addUtilities to add some new utilities, like this:

addUtilities(
  {
    'select:is(.claris-select, .claris-multiselect) option:checked': {
      '@apply !bg-[image:linear-gradient(var(--color-brand-100),var(--color-brand-100))] !text-gray-750':
        {},
    },
  },
  ['responsive']
);

…I get this error letting me know that the class doesn't exist and that if it's a custom class, I'll need to define it in a @layer directive:

[Error] Tailwind CSS: <css input>: The `!text-gray-750` class does not exist. If `!text-gray-750` is a custom class, make sure it is defined within a `@layer` directive.
      CssSyntaxError: <css input>: The `!text-gray-750` class does not exist. If `!text-gray-750` is a custom class, make sure it is defined within a `@layer` directive.

I'm not sure if that would be a straightforward change or not to get the classes listed in @apply blocks within addUtilities/matchUtilities to account for other classes created using addUtilities/matchUtilities, which is what I presume the issue here, but that would be a huge benefit. Otherwise, the path of least resistance is probably to go Adam's route and just refactor tailwind-lerp-colors as a function that can be used to wrap around any group of colors and interpolate on that group.

It certainly wouldn't be my preferred solution, as it wouldn't constitute as a true Tailwind plugin, but if it works, it works. Let me know what you think. Thanks! 🙏🏼

brandonmcconnell commented 2 years ago

Also, the reason it probably isn't working with your setup is that my plugin is looking at theme.colors and theme.extend.colors (not theme('colors')), which appears to be empty in your setup, which would require you to employ the includeBaseColors property.

I also need to include an additional property to determine whether to include legacy color names or not to silence the related warnings (e.g. As of Tailwind CSS v3.0, color1 has been renamed to color2), but we can ignore those warnings for now.

If you think there's a chance Tailwind could make a change to account for dynamically generated classes within @apply blocks with other addUtilities/matchUtilities, let me know and I'll take a deeper look into your example.

Thanks again!

bradlc commented 2 years ago

Hey @brandonmcconnell. @RobinMalfait figured out why I was struggling to get the plugin (v1.1.0) to work. It seems that the plugin does not work when executed only once. We can get the plugin to work by running a watcher (npm run build -- --watch) and saving the tailwind.config.js file. I think this solidifies what Adam said about your approach in v1.1.0: that when it does work it's only by chance. For this reason I don't consider there to be an issue with IntelliSense and I'm going to close this issue.

Regarding your other approaches, after discussing with the team I think that Adam's original assessment is accurate: "sorry that it doesn't work but we didn't intend for it to work and not interested in working on it" – sorry about that. For what it's worth I personally think that Adam's suggestion of a function that generates a color object makes the most sense:

// tailwind.config.js
const lerp = require('tailwindcss-lerp-colors')
const colors = require('tailwindcss/colors')

module.exports = {
  theme: {
    colors: lerp(colors)
  },
  plugins: [],
}

Sorry this isn't the result you were hoping for but I hope you can land on a solution that you're happy with 🙏

brandonmcconnell commented 2 years ago

@bradlc @RobinMalfait @adamwathan Thanks for looking into that for me!

I followed Adam's suggestion and refactored tailwind-lerp-colors to operate as a function wrapping theme.colors instead of a plugin, and it's working great.

In case any of you are interested in reading up on the update(s) I made to tailwind-lerp-colors which resolved this issue, please see the release notes for versions v1.1.3 and v1.1.4 and the updates top the README (file / diff).

I would love to see Tailwind grow to a place where this sort of added functionality is possible to achieve using Tailwind's built-in plugin infrastructure, though I understand there's not a real need for that at this point, especially alongside everything else Tailwind is currently working on. For now, this solution works great, so thank you for the suggestion!

Please let me know if you have any other input or suggestions on how I could build this to further scale with Tailwind.

Cheers, and thanks again 🌮🥂