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

Automated space adding to look-like-mathematic-operators leads to multiple errors #8661

Closed
DanielKraemer opened this issue Jun 17, 2022 · 11 comments · Fixed by #8688
Closed
Assignees

Comments

@DanielKraemer
Copy link

DanielKraemer commented Jun 17, 2022

The function "normalize" with dataTypes.js leads to errors when using with some plugins like plaiceholder within 11ty for example.
https://github.com/tailwindlabs/tailwindcss/blob/master/src/util/dataTypes.js Line 54

grafik

The marked line adds spaces around look-like-mathematic-operators.

We're using plaiceholder to get blurred preview images and got filePaths like "3-questions-to" or "TEXT-2/TBC" we got a the file path "3 - questions-to" or "TEXT-2 / TBC". And so this isn't a valid file path.

And so this is useless for us. And there is no workaround. I wasted 8+ hours trying to fix this garbage which seems to be useless to me and is only helpful for a better look in the rendered css.
But when I fix this, I got another errors with plaiceholder and fs...

Is there any case we need this or is it only for a better look?

@DanielKraemer
Copy link
Author

I tested this again.
While doing a mathematic calculation you need the space before and after the operator.
But without a css math function like calc, min, max, clamp for example the calculation won't work

So this is useless or am I completely wrong

@DanielKraemer
Copy link
Author

If I got a "go" by any main developer I'd change or remove this line myself so that it returns the original value and open a pull request

@adamwathan
Copy link
Member

adamwathan commented Jun 17, 2022

First of all I understand that it's frustrating when you run into a bug, but opening an issue that ignores the issue template, doesn't include a reproduction, complains about how we wasted your time, and goes on to talk about code in the project being "garbage" isn't the sort of behavior I'm personally willing to put up with. This is a free, MIT-licensed, open source project which means there is a workaround — you simply fork the code and change it to do what you want. When you open an issue asking for a bug to be fixed, you're asking for a favor, and no one is obligated to fix it, as a consumer of open source you have to be okay with that.

That said, I looked into this briefly and it looks like it was introduced in this PR:

#8091

The idea was to fix some other bug while preserving the existing behavior of adding spaces in situations like aspect-[16/9]. It does look like the spaces aren't actually needed by the browser though so it probably is safe to remove.

Before we do that, can you provide a reproduction of the problem? I'm guessing it's in situations like bg-[url('...')] or something where there is a path in an arbitrary value? We have code in the framework that's intended to bypass these types of normalizations in URLs already so I want to make sure I understand why you're hitting that part of the code before we just throw something out.

@DanielKraemer
Copy link
Author

@adamwathan first I have to be sorry for using such a rude language.
Usually this is not my style and I think I was too emotional this morning while writing this issue

2nd: Next time I'll use the issue template to help you reading this issue easier.

Thank you for your detailed explanation and the given pull request. Yes, this could be case for this usage.

I'll give you a short explanation of my problem.

We're using 11ty for creating a full static blog, tailwindcss for the layout and plaiceholder for creating blurred images as placeholder till the original pictures are rendered/loaded completely

As 11ty isn't currently able to work with async function, we're trying to use plaiceholder with tailwind as described on this page: https://plaiceholder.co/docs/plugins/tailwind

So we got class names like "plaiceholder-[/3-questions-to/file.png]" or "plaiceholder-[/filepath-2/filename.png]"
The described problem converts this paths to "/3 - questions-to/file.png" or "/filepath-2 / filename.png".
So the files cannot be found.

If I try to escape this with backslashes, so that I got "/3-questions-to/file.png" for example, but then I got error from fs that this isn't a legal path (this message is correct)
Then I tried working with "url(file://)", but I got an error with plaiceholder again that this path doesn't start with a leading slash what is required by plaiceholder. So I turned into a loop of trouble..

The next screenshot is from the plaiceholder node_modules
grafik

If we could talk about how to fix it, I'd try to support and writing this myself an putting it into the main code via a pr

@adamwathan
Copy link
Member

Hey! Really need an actual reproduction here I think, this would be a lot easier to troubleshoot if you would provide an actual GitHub repository I can clone that demonstrates the problem, and is why we require reproductions as part of the issue template.

Trying to set up a reproduction for this myself and it's proving painful because this plugin doesn't work with Tailwind CSS v3, only v2 with JIT mode enabled. I've got it basically running and I can't reproduce the issue myself, the CSS is generated with no spaces added:

image

I'm still not sure what the issue is really though since you haven't provided a reproduction, or explicitly stated what you expect the output to be.

Here's my reproduction:

https://github.com/adamwathan/tailwind-repro-8661

Have to use npm i --force to install it because the Plaiceholder plugin has a peer dependency on Tailwind CSS v2 which is no longer developed, then run npm run build to generate the CSS.

If you can provide an actual reproduction and a very clear description of the expected output as compared to the output you're seeing it will be easier to help 👍🏻

@DanielKraemer
Copy link
Author

@adamwathan perfect, thanks😀

As I cannot send you link to our repo directly cause this is hosted on our in-House gitlab server, I'll try to rebuild a testing case here on GitHub this weekend.

I'll then provide the link to it as a comment when I finished it

@DanielKraemer
Copy link
Author

@adamwathan I created the test repository as I proposed before.
https://github.com/DanielKraemer/11ty-tailwind-plaiceholder-filepath-spaces-error

When running npm run start you shold get an error like this:
grafik

If you got some questions don't hesitate contacting me

@thecrypticace thecrypticace self-assigned this Jun 20, 2022
@thecrypticace
Copy link
Contributor

Minimal reproduction: https://play.tailwindcss.com/IAwqejTC8S?file=config

@thecrypticace
Copy link
Contributor

thecrypticace commented Jun 20, 2022

The main problem here is that we do preserve url-like-things but only if they're inside a url(…). The @plaiceholder/tailwindcss plugin is taking the value we've normalized because it's being treated as a raw CSS expression which isn't what we want here.

I've opened a PR to resolve this by just not adding spaces around the / when not inside math / calc / etc… functions

@DanielKraemer
Copy link
Author

@thecrypticace thank you very much for fixing this.
But I hope that this doesn't conflicts with #8091
Is there a correct way to support you with e. g. code on some other issues?
Maybe I can support a bit

@thecrypticace
Copy link
Contributor

It shouldn't be an issue. I just left the spaces around the / originally because I wasn't sure if there'd be any fallout but there shouldn't be any problems from this.

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 a pull request may close this issue.

3 participants