Skip to content
This repository has been archived by the owner on May 14, 2021. It is now read-only.

How to fix all these interpolation edge cases? #68

Closed
emilgoldsmith opened this issue Jul 10, 2017 · 2 comments
Closed

How to fix all these interpolation edge cases? #68

emilgoldsmith opened this issue Jul 10, 2017 · 2 comments
Milestone

Comments

@emilgoldsmith
Copy link
Member

(first of all, hope it's all good I added a discussion label)

So, there are obviously heaps of interpolation edge cases, I think all issues labeled with bug other than #26 are currently due to this, and I listed 4 cases (some of them overlapping with already existing issues) in the comments of #63. So I think we should have a bit of a discussion about how to fix this if you're up for it @styled-components/stylelint-processor.

I see the biggest issues with this as:

  • Many things will simply be unknowable before runtime such as what the props will be to a component etc.
  • If we were to try doing some smart stuff with interpreting Javascript to figure out the parts we could from pre-runtime it would probably take a lot of man-hours and could also potentially really damage our performance in quite a few cases.
  • Currently we are enumerating cases based on assumptions of use, and it's always a difficult path to for a proof (yep, I think like a mathematician 😛), and therefore for finding a bug-free missed-edge-case-free approach as well.

An idea came to me while pondering about the different things I've been working on in this repo recently, that should definitely work, and not be too hard to implement. I also don't think it would hinder UX too badly, but that's definitely up for discussion. It's definitely not a dream-worthy perfect solution as the user would have to do a bit of work, but let's see what you think, and otherwise that's also why the discussion tag is here!

So my approach would:

  • Ditch any attempts at trying to interpret JS, and continue on the path of inserting dummy values that will stop Stylelint from complaining, and then interpolations will either be linted where it was defined if defined with a styled-components helper, or not at all. It will hopefully do it in a much more comprehensive and correct way though, so we could close all those bug labeled issues all at once 😀
  • Make the user declare what kind of output this interpolation will have at the start of every interpolation in a comment, and we can then easily substitute the right dummy value
  • In case the user doesn't declare the type we can keep defaults such as something what we're using now, and also of course let that be known, so you would only have to declare in special cases and we can hopefully guess for the rest like we are doing now.

So implementation wise I would do something like this:

  • I would plan on using something like this css vocabulary list and not needing people to type it all out, but maybe make some shortcuts and if a substring has a unique match then use it etc. maybe store it as a trie or something. This would make sure we covered basically all edge cases, and the few that would possibly arise after should not be so numerous that it would feel bad adding a few more special-cases.
  • For finding the comments I already checked this AST explorer where you can see we would be able to find the comments easily by looking at program.body[0].expression.quasi.expressions[0].leadingComments[0].value
  • I would maybe prefix the comment like other operational comments do such as stylelint-enable and eslint-enable, though I would try and make it shorter, maybe an abbreviation such as scp (Styled Components Processor)
  • An actual example that would for example fix Doesn't properly handle contextual selectors #34 and Strange errors with refering to other components #54 would be:
export const ButtonGroup = styled.div`
  white-space: nowrap;

  > ${/* scp-sel */ Button} {
    margin: 0;
    border-radius: 0;
    vertical-align: top;
  }
`

Or you could of course also in that case write out scp-selector if you wanted.

  const Button = styled.button`
    margin-${/* scp-custom-right */ props => ((props.theme.dir === 'rtl') ? 'left' : 'right') }: 12.5px;
  `;

So that's my speech! I know it got a bit long. It could possibly use a bit of refining which is also why I'd love your feedback and thoughts, and maybe we'll end up going in a completely different direction, but let's try starting a discussion as this is definitely an important problem that needs a very solid solution.

@mxstbr
Copy link
Member

mxstbr commented Jul 10, 2017

I love this, sounds like a great plan. I agree that trying to interpret JavaScript further will not go anywhere, I really like the sound of this. Let's get it done!

@mxstbr
Copy link
Member

mxstbr commented Aug 28, 2017

Done in #70! (not closed because it was merged into v1 rather than master, so manually closing)

@mxstbr mxstbr closed this as completed Aug 28, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants