Custom spacings not working

[MUppenkamp]

Hello to you 🙂

I am happily using your plugin in my private and work projects. At work, we've been using it for almost 2 years now and it works perfectly. But now with the update to v4 an error occurred.
In our project we have various customized spacings. For example --spacing-10l or --spacing-type-area which each holds a calc value. The error that occurs is the following:

I have cloned your project and tracked down where this happens. When we take a look at the inlineSize.js file (but also any other case doesn't work as well) we'll see this for-loop. When we take --spacing-type-area as our example the theme("width") returns 'type-area': 'calc(var(--spacing) * 35)'. In the if-clauses this will go into the else case creating the error.

for (const property in values) {
    if (keywords.includes(property)) {
      calculatedValues[property] = values[property];
    } else if (property === "px") {
      calculatedValues[property] = "1px";
    } else if (property.includes("/")) {
      calculatedValues[property] = `calc(${property} * 100%)`;
    } else {
      calculatedValues[property] = `calc(var(--spacing) * ${property})`;
    }
  }

I build a fix with a colleague of mine and this is our suggestion:

  for (const property in values) {
    if (keywords.includes(property) || (!/^\d+(?:\.\d+)?$/.test(property) && values[property])) {
      calculatedValues[property] = values[property];
    } else if (property === "px") {
      calculatedValues[property] = "1px";
    } else if (property.includes("/")) {
      calculatedValues[property] = `calc(${property} * 100%)`;
    } else {
      calculatedValues[property] = `calc(var(--spacing) * ${property})`;
    }
  }

The regular expression /^\d+(?:\.\d+)?$/ matches a positive whole number or decimal number. This is the normal case for the spacings coming from tailwind. In our case we need to use the value that's defined for the spacing and not the name itself.

Your tests also keep working and I tested it with the normal tailwind setup without any configurations and everything works perfectly!

This change would be added in these files:

Would you be so kind to have a look at this? I couldn't create a pull request but I'd be very happy to do so if you approve 😊

Best regards,
Mona

[stevecochrane]

Hi @MUppenkamp,

I'm glad this has been useful to you, and thank you so much for the detailed explanation and proposed fix! This would also potentially fix the other open issue, where someone else was trying to use custom decimal values and it no longer works after v4.

Unfortunately I can't quite get this working locally yet... I tried updating inlineSize.js with your fix and the plugin still works just fine, but I still can't seem to generate any new utilities. For example if I try to add the following to the inlineSize.html test template...

    <div class="is-97"></div>
    <div class="is-type-area"></div>
    <div class="is-7.5"></div>

...I still don't see any classes being generated for these in the test output. Maybe the way I run the tests differs from real world usage, though. Do you have an example project with a working fix (or one that I can locally apply your fix to) that I can try, perhaps? Also you're totally welcome to file a pull request if that is easier, and if there's anything I've misconfigured with the repository that prevents new pull requests, please let me know and I'll take care of it.

Thank you!
Steve

[MUppenkamp]

Hii @stevecochrane !
Thanks for your answer 😊 This is the problem I encounter when I want to push the feature branch I made

I haven't really take a look at the tests and how they work (I just assured that they work like before with your implementation 😄) but here is a short insight in the config from my project and the output int the inlineSize plugin part.

@theme {
  --spacing-type-area: calc(var(--spacing) * 35);
}

and when I log the output from calculatedValues you can find 'type-area': 'calc(var(--spacing) * 35)'

And when using it it looks like this:

I am not sure if I understood you correctly but with my fix and my config it works locally right now!

Mona

[MUppenkamp]

An addition: The problem with the other issue is sadly not fixed with this. I tested it and it seems like any other value that isn't predefined by tailwind is also not working. When I log these const values = theme("width"); you can find these

But if I e.g. try to use is-100 it seems to not be working as well

[stevecochrane]

Hi @MUppenkamp,

I'm able to run it locally now, thank you! I hadn't added to @theme {} before and that's why the new class wasn't showing up in the output, but now it works and I understand the changes. This would definitely be valuable to have in the project and I'd be glad to put out a new minor release for it.

Also I see now why putting up a PR hasn't been working: contributors don't have write access to this project (unless I explicitly allow them one-by-one) so if you want to file a PR you will need to make a fork of this project first. Once you do that and make a branch on your fork with your changes, then you should be able to file the PR.

If you would like to do that, great (and you would appear under Contributors on the project too) but if that's too much I would also be glad to make a PR myself, apply the changes to the files you mentioned and share it with you for approval before merging. I'll also add a new contributors section to the README to give you credit that way if we take that approach.

I should warn you that the tests will probably break, and updating them is a huge pain that I wouldn't wish on anyone else, but we'll see how it goes and I'll update the tests myself when needed.

Thanks again!
Steve

[t0byman]

@stevecochrane Maybe you can copy part of this https://github.com/parcel-bundler/lightningcss/blob/8fa65926584075e2f76357910001a19b6efe9c35/CONTRIBUTING.md for your own CONTRIBUTING.md

[MUppenkamp]

Hello @stevecochrane,
I'd be happy to do this! I will open a Pull request this week! I might need help with the tests probably, but we'll see!