Make -styled-mixins unique and clarify interpolation linting in readme

[emilgoldsmith]

Up until now I just added a failing test for #55. It seems to be a problem with this line returning undefined on a string as opposed to a variable.

I'll probably look at it again tomorrow night after work, and see if I can also fix it, my first impression is that it's a bigger problem with the approach and that it's better to find a better approach rather than patching an edge case for another one to soon pop out, but again, I haven't looked at it that much yet.

Any advice from more seasoned contributors that know the codebase better is of course welcome, otherwise I'll look at it again tomorrow night.

(fixes #55)

[emilgoldsmith]

Hmm... What's up with the oneliner and the random line collections in my commit? Is that what husky was doing with the pre-commit stuff? Definitely wasn't what I was doing :P. How can I avoid that?

[emilgoldsmith]

Just had a bit of a think about it on the train to work again.

My first thought for a quickfix would be doing this in tagged-template-literal.js:

let count = 0; // new
/**
 * Merges the interpolations in a parsed tagged template literals with the strings
 */
const interleave = (quasis, expressions) => {
  let css = ''
  for (let i = 0, l = expressions.length; i < l; i += 1) {
    const prevText = quasis[i].value.raw
    const nextText = quasis[i + 1].value.raw
    const expression = expressions[i]

    css += prevText
    if (isLastLineWhitespaceOnly(prevText)) {
      css += `-styled-mixin${count}: ${expression.name}` // changed
      if (nextText.charAt(0) !== ';') {
        css += ';'
      }
    } else {
      css += `$${expression.name}`
    }
  }
  css += quasis[quasis.length - 1].value.raw
  return css
}

It's a bit of a quickfix but I guess it keeps your current approach alive?

I'm guessing there isn't any smart library that could handle actually evaluating the interpolations for us? We couldn't evaluate the strings first with the actual styled library or something? Would of course be awesome but I can see it could also be a big amount of work as you'd have to parse imports etc.

[ghost]

Hmm... What's up with the oneliner and the random line collections in my commit? Is that what husky was doing with the pre-commit stuff? Definitely wasn't what I was doing :P. How can I avoid that?

That's because of prettier. Looks like a bug.

edit: Might be related to prettier/prettier#2291

[ghost]

my first impression is that it's a bigger problem with the approach and that it's better to find a better approach rather than patching an edge case for another one to soon pop out, but again, I haven't looked at it that much yet.

I think so too. For now I haven't had enough time to really consider which approach would be the most maintainble, but I'm also expecting that a slight refactor to clean things up a bit would be good. I'm intending to make things a bit more modular to prevent things from getting too complex.

I'm guessing there isn't any smart library that could handle actually evaluating the interpolations for us? We couldn't evaluate the strings first with the actual styled library or something? Would of course be awesome but I can see it could also be a big amount of work as you'd have to parse imports etc.

That might be possible in some cases, but ultimately I think it would be quite difficult. Some values would only be available at runtime if I'm not mistaken and there would probably be quite a performance hit.

[emilgoldsmith]

That might be possible in some cases, but ultimately I think it would be quite difficult. Some values would only be available at runtime if I'm not mistaken and there would probably be quite a performance hit.

Yeah I agree to that. Could definitely make a big performance hit. It is a bit sad you can't have your interpolations linted as of right now though, I might try looking into if there is some smart library we could use to handle some of the cases as I believe most cases should be known at compile time, and maybe also looking into different approaches and get back to you here.

[ghost]

Yeah I agree to that. Could definitely make a big performance hit. It is a bit sad you can't have your interpolations linted as of right now though, I might try looking into if there is some smart library we could use to handle some of the cases as I believe most cases should be known at compile time, and maybe also looking into different approaches and get back to you here.

Great! Thanks for taking a look at it, appreciate the help 👍

[emilgoldsmith]

edit: Might be related to prettier/prettier#2291

Yeah seems very likely this is the problem. I'll add some // prettier-ignore statements.

Great! Thanks for taking a look at it, appreciate the help 👍

No problem! I already took a bit of time to look into it now actually, and I'll be pushing up my idea of a fix + commenting my reasoning soon :).

[coveralls]

Coverage increased (+0.004%) to 99.379% when pulling b0e2bd1 on emilgoldsmith:fix/#55/several-interpolation-strings into 834eb63 on styled-components:master.

[coveralls]

Coverage increased (+0.004%) to 99.379% when pulling 2012bce on emilgoldsmith:fix/#55/several-interpolation-strings into a4ce9db on styled-components:master.

[coveralls]

Coverage increased (+0.004%) to 99.379% when pulling 2012bce on emilgoldsmith:fix/#55/several-interpolation-strings into a4ce9db on styled-components:master.

[coveralls]

Coverage increased (+0.004%) to 99.379% when pulling 2012bce on emilgoldsmith:fix/#55/several-interpolation-strings into a4ce9db on styled-components:master.

[emilgoldsmith]

Okay, a few things:

1. Force pushed the first commit to add // prettier-ignore statements and also rebased onto newly pushed code from Fixed multiple interpolations in a property #52 etc.

2. I looked into actually parsing a bit, and also just thought about it. While I believe in practice we could probably solve many cases as mostly people might just import mixins from a single file that they store it in / substitute variables declared in that same file, the worst case is of course that we'd have to emulate a full webpack build basically which would of course be horrendous for performance. And even with the things we could possibly support, it would still be a bigger project, so if you're okay with it @ismay and maybe if you have an issue tag for "long term goals" or something of equivalent meaning, I'll open an issue on that though it is probably low priority. It could also possibly be something that could be a goal for my webpack linter.
   To reflect the "giving up" on that I added a piece to the README explaining that we don't support interpolation linting so people won't be surprised by that.

3. My solution which I think is good enough for now, especially given that we add a piece to the README is just making all the -styled-mixins unique by adding a count. And then I just changed the expression.name to dummyValue because I think that reflects the actual use of it better? I mean, since the expression.name wasn't really being used for anything other than a placeholder to make stylelint pass over it quietly, right? If I missed a usecase for the expression.name please say so, but I also thought since it returns undefined on anything that's not a variable I'm assuming it didn't have any other uses.

Thoughts? :)

[emilgoldsmith]

Also, from my POV this is ready to merge as of now, pending your comments.

[mxstbr]

LGTM, thanks!

[mxstbr]

Great work! 👌

[emilgoldsmith]

Thanks! And thanks for the invite :). Look forward to working with you in my spare time :).

[mxstbr]

Thanks for the great work so far, happy to have you on board!