tailwindlabs / tailwindcss

A utility-first CSS framework for rapid UI development.
https://tailwindcss.com/
MIT License
82.47k stars 4.17k forks source link

e() for escaping is broken #509

Closed lbjones closed 6 years ago

lbjones commented 6 years ago

I upgraded from 0.5.2 to 0.6.1 and the e() function is now broken (or changed possibly).

e(50) // used to output '50', now it outputs '\35 0'

I know that 50 doesn't need to be escaped, but I don't always know what a user might enter.

adamwathan commented 6 years ago

What is actually broken? That output still works:

https://jsfiddle.net/x462ybws/

The problem is CSS classes are not allowed to start with a number, so if you try to escape a string that starts with a number, it's going to escape that number. If you prefer it not to be escaped (even though everything still works fine), escape the whole class string including whatever prefix you are adding:

e(`my-class-50`)
lbjones commented 6 years ago

Thanks for the quick reply!

Hmm very interesting. Ok so I didn't describe the properly, sorry about that.

I generated that output ('\35 0') using a standalone project using the tailwind cli.

It's not working in my project using the vue cli. (I also upgraded that from rc2 to rc3).

Here is the code I'm using:

    // Class name: .pin-{side}-{name}
    // Example: .pin-t-50  => top: 50px;
    // Example: .-pin-b-50 => bottom: -50px
    require('./pin')({
      positive: {
        '25': '25px',
        '50': '50px',
        '100': '100px',
      },
      negative: {
        '25': '-25px',
        '50': '-50px',
        '100': '-100px',
      },
      variants: ['responsive'],
    }),

and the plugin code:

module.exports = function({ positive, negative, variants }) {
  return function({ addUtilities, e, config }) {
    const utilities = [];
    Object.entries(positive).map(([name, value]) => {
      utilities.push(['top', 'right', 'bottom', 'left'].map(side => ({
        [`.pin-${side.charAt(0)}-${e(name)}`]: {
          [side]: `${value}`
        }
      })));
    });
    Object.entries(negative).map(([name, value]) => {
      utilities.push(['top', 'right', 'bottom', 'left'].map(side => ({
        [`.-pin-${side.charAt(0)}-${name}`]: {
          [side]: `${value}`
        }
      })));
    });
    addUtilities(utilities, variants);
  }
}

And some of the output css:

.pin-b-35 0 { // not escaped!
   bottom: 50px;
}

Also I've noticed that when I update the plugin or tailwind.js using the 'npm run serve' command, the code will recompile on save, but I need to restart the process in order for the changes to take effect.

Thanks for the workaround. Since these are all integers anyway I don't need to escape them.

I'll need to dig into it more to determine if this is a problem with the vue cli, sounds like it is.

lorlab commented 6 years ago

Not sure if related, but I am also experiencing problems after updating to 0.6, specifically with the Aspect Ratio Tailwind Plugin, which was working correctly before updating Tailwind.

The classes the plugin generates should be

.aspect-ratio-square { padding-top: 100%; }
.aspect-ratio-16/9 { padding-top: 56.25%; }
.aspect-ratio-4/3 { padding-top: 75%; }
.aspect-ratio-21/9 { padding-top: 42.86%; }

But instead they are:

.aspect-ratio-square { padding-top: 100%; }
.aspect-ratio-31 6\/9 { padding-top: 56.25%; }
.aspect-ratio-34 \/3 { padding-top: 75%; }
.aspect-ratio-32 1\/9 { padding-top: 42.86%; }
adamwathan commented 6 years ago

@lbjones I just tested your plugin code myself and everything comes out escaped as expected:

image

Alternatively though if that's not working for whatever reason, you can rewrite your plugin like this to prevent escaping those numbers unnecessarily:

module.exports = function({ positive, negative, variants }) {
  return function({ addUtilities, e, config }) {
    const utilities = [];
    Object.entries(positive).map(([name, value]) => {
      utilities.push(['top', 'right', 'bottom', 'left'].map(side => ({
        [`.${e(`pin-${side.charAt(0)}-${name}`)}`]: {
          [side]: `${value}`
        }
      })));
    });
    Object.entries(negative).map(([name, value]) => {
      utilities.push(['top', 'right', 'bottom', 'left'].map(side => ({
        [`.${e(`-pin-${side.charAt(0)}-${name}`)}`]: {
          [side]: `${value}`
        }
      })));
    });
    addUtilities(utilities, variants);
  }
}

Note that I'm escaping the entire class name, not just the user provided portion. That's what I was talking about in my first comment.

adamwathan commented 6 years ago

@lorlab Can you share a project that reproduces that? In my testing the classes are properly escaped:

.aspect-ratio-square { padding-top: 100%; }
.aspect-ratio-\31 6\/9 { padding-top: 56.25%; }
.aspect-ratio-\34 \/3 { padding-top: 75%; }
.aspect-ratio-\32 1\/9 { padding-top: 42.86%; }

I'm going to make a PR to that plugin anyways to avoid this escaping since it's unnecessary, but it appears to work fine still here.

lorlab commented 6 years ago

My project is using nuxt.js but I have just tested the plugin with a new vue project created with the latest vue-cli 3. I just installed tailwindcss and tailwindcss-aspect-ratio and built the project. The classes generated by the plugin have the wrong escaping like I showed above. Since @lbjones is also using vue-cli maybe it has something to do with it?

Edit: I tried manually making the change in your PR to the plugin, and as expected it solves the problem.

adamwathan commented 6 years ago

Yep okay tried it in a vue-cli project and can confirm something weird happens where it unescapes those characters. Also confirmed that vue-cli does handle it correctly if you actually do have a class that starts with a number (it preserves the escaping).

I've confirmed even if you add these styles manually, the build process removes the slash in front of the first number for some reason:

.aspect-ratio-\32 1\/9 {
  padding-bottom: 42.8571429%;
}

No idea where or why that is happening 🤔

Either way, it's definitely annoying that this is a break, but if you use the escape function the way I've explained in this issue instead of just to escape the modifier, everything does work ok.

tlgreg commented 6 years ago

@adamwathan Didn't look into it, but isn't it a linter maybe trying to fix it? It might catches it as an unnecessary escape before a number.

adamwathan commented 6 years ago

@tlgreg Yeah could totally be, if so it's a bug with whatever is trying to fix it because \32(space) unescapes to 2 in the browser, not 32(space) 😕

adamwathan commented 6 years ago

I've opened an issue with vue-cli because it's definitely a bug on their end (it happens even if you aren't using Tailwind) so I'm going to close this issue.

I've also expanded on the docs for the escape function to make it more clear that you should escape entire class names (not just portions of class names) to get the most optimized output:

https://tailwindcss.com/docs/plugins/#escaping-class-names