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

Highlighting breaks when using attrs with props value #292

Open
panda919 opened this issue May 19, 2021 · 25 comments · May be fixed by #300
Open

Highlighting breaks when using attrs with props value #292

panda919 opened this issue May 19, 2021 · 25 comments · May be fixed by #300

Comments

@panda919
Copy link

panda919 commented May 19, 2021

**Describe the bug **
I gonna apply attris by pass props.
But highlight is broken.

Screenshot

image

Expected behavior
HightLight has to be keep

  • OS: [Windows10]
  • VSCode Version: [1.56.0]
  • Extension Version [1.6.1]

Additional context
Add any other context about the problem here.

@panda919 panda919 added the bug label May 19, 2021
@jasonwilliams
Copy link
Collaborator

@panda919 can you please provide some copyable code with this

@panda919
Copy link
Author

panda919 commented May 20, 2021

I try it on react native
import { View, Text, TextInput, ScrollView } from 'react-native';
`const hPaddingStyle = { paddingHorizontal: Metrics.medium };
const vPaddingStyle = { paddingVertical: Metrics.small };
const hMarginStyle = { marginHorizontal: Metrics.medium };
const vMarginStyle = { marginVertical: Metrics.small };
const fullWidthStyle = { width: '100%' };
// Adapting based on props

export const RootBox = styled.View.attrs(props => ({
style: {
...(props.hPadding && hPaddingStyle),
...(props.vPadding && vPaddingStyle),
...(props.hMargin && hMarginStyle),
...(props.vMargin && vMarginStyle),
...(props.full && fullWidthStyle),
...(props.aspectRatio && { aspectRatio: props.aspectRatio }),
...props.style,
},
}))opacity: ${props => props.opacity || 1};;

export const ColumnBox = styled(RootBox)flex-direction: column;;`

@mateusrachid
Copy link

Also breaks with this:

import React from 'react';
import styled from 'styled-components';

const Root = styled.div.attrs(props=>({
  'data-status': `${props.status}`
}))`
  display: block;
  height: 100%;
`;

export default function MyComponent({...props}){
  
  return (
    <Root {...props}>
      <span>Hello world!</span>
    </Root>
  );
}

image

@mateusrachid
Copy link

Seems like the problem appears when the argument to the attrs method contains parenthesis.

It breaks with things as simple as:

const MyComponent = styled.div.attrs(({}))` `;
// or
const MyComponent = styled.div.attrs(()=>({}))` `;
// or
const MyComponent = styled.div.attrs((someVar))` `;
// or 
const MyComponent = styled.div.attrs(someFunc())` `;

But does not break with:

const MyComponent = styled.div.attrs(()=>{
  return {};
})` `;

// nor with

const MyComponent = styled.div.attrs(someVar)` `;

@panda919
Copy link
Author

At least, I need to apply dynamic style by pass props

@jasonwilliams
Copy link
Collaborator

Would someone be able to work on this? We have a contributing guide that’s helpful

@panda919
Copy link
Author

Honest, I didn;t get this issue on previous versions.

@bluenex
Copy link

bluenex commented May 25, 2021

I have narrowed down the change and found that the bug is introduced since 1.6.0.

I believe it is from #287

There is a pattern for attrs on line 159, but }> was replaced by (?:}>|\\)\\)) on line 7. One thing to notice is that, style syntaxes after .attrs() are colored correctly. However, all styled syntaxes after the one with .attrs() are not colored correctly. I guess all the lines after somehow match line 7 instead of their correct pattern?

I got the purpose of changing line 7 like that, but not sure where to move the new pattern to. Any guide and I may open the PR for this?

PS. my workaround for now is to rollback to version 1.5.2

@jasonwilliams
Copy link
Collaborator

jasonwilliams commented May 25, 2021

I have narrowed down the change and found that the bug is introduced since 1.6.0.

I believe it is from #287

There is a pattern for attrs on line 159, but }> was replaced by (?:}>|\\)\\)) on line 7. One thing to notice is that, style syntaxes after .attrs() are colored correctly. However, all styled syntaxes after the one with .attrs() are not colored correctly. I guess all the lines after somehow match line 7 instead of their correct pattern?

I'm got the purpose of changing line 7 like that, but not sure where to move the new pattern to. Any guide and I may open the PR for this?

PS. my workaround for now is to rollback to version 1.5.2

Great investigation @bluenex yes I think you’re correct.
Out of interest what example code are you using for your observation? It may be worth sharing so we’re seeing the same thing. I don’t know why sometimes line 7 matches without doing a deep dive. Have you looked at the contributions guide or are you already set up?

@bluenex
Copy link

bluenex commented May 26, 2021

@jasonwilliams I have already set up and ran the test. Here is a snippet to reproduce the behavior and found that the affected code are anything after the component with .attrs() especially JSX. I feel this is a conflict with JSX highlighter? Here is an example of incorrectly colored code that can be reproduced in version 1.6.3:

import React, { useState } from 'react';
import styled, { css } from 'styled-components';

const WithoutAttrs = styled.div`
  /* styled here are colored correctly */
  color: red;
  background-color: white;
  padding: ${(p) => p.color};
`;

const WithoutAttrs2 = styled.div`
  /* styled here are also colored correctly */
  color: red;
  background-color: white;
  padding: ${(p) => p.color};
`;

const WithAttrsWithReturnInCurly = styled.div.attrs(() => {
  return { };
})`
  /* styled here are still colored correctly */
  color: red;
  background-color: white;
  padding: ${(p) => p.color};
`;

const WithAttrsWithShortHandReturn = styled.div.attrs(() => ({ }))`
  /* styled here are still colored correctly */
  color: red;
  background-color: white;
  padding: ${(p) => p.color};
`;

// anything after this is colored incorrectly
// in my case keyword const and especially jsx are not colored correctly
const variable = true;

const NotEvenStyledComponent = () => {
  // non jsx not affected much
  if (variable) {
    console.log('test');
    return undefined;
  }

  return null;
};

const StyledDiv = styled.div`
  /* styles seem to be colored correctly */
  color: red;
  background-color: white;
  padding: ${(p) => p.color};

  ${(p) => p.margin && css`
    margin: 10px ${p.margin}px;
  `}
`;

// following JSX is completely messed up
const SomeSmallComponent = () => (
  <div>
    <h1>This is a demo component</h1>
    <p>this is a demo paragraph</p>
  </div>
);

export default () => {
  const [state, setState] = useState();

  return (
    <StyledDiv>
      <SomeSmallComponent />
      <div>
        <p>Still incorrect</p>
      </div>
    </StyledDiv>
  );
};

Screenshots

1.5.2 1.6.3
image image

@jasonwilliams
Copy link
Collaborator

jasonwilliams commented May 26, 2021

@bluenex that was super helpful thanks, I should be able to use the same template to reproduce.

the first step would be to see why my last change causes that. It most likely needs removing then adding back in in a more specific way. Then we add a test to capture.

@jasonwilliams
Copy link
Collaborator

@bluenex do you have a conflicting extension? Im not seeing the same issue you are (against master)

image

@bluenex
Copy link

bluenex commented May 27, 2021

@bluenex do you have a conflicting extension? Im not seeing the same issue you are (against master)

I think you do have the issue. Check the color of const before line 27 and after, they are different color. In the JSX below, the opening tag and closing tag also have different color.

I use GitHub Dark theme so the highlight color is slightly different.

@novli
Copy link

novli commented May 27, 2021

@jasonwilliams Please, check the file extension. Obviously, .ts vs .tsx (.js vs .jsx) have different highlighting rules by default.

@jasonwilliams
Copy link
Collaborator

@jasonwilliams Please, check the file extension. Obviously, .ts vs .tsx (.js vs .jsx) have different highlighting rules by default.

@novli my issue was the theming not the file extension. It’s very slight but I can see the issue now

@jasonwilliams
Copy link
Collaborator

jasonwilliams commented May 28, 2021

@bluenex I've made progress by reverting my changes and making a specific block to capture the double parens. A draft PR is open

You will see that with your example above it works.
image

The downside is its not perfect and causes issues with this block.

const WithAttrsWithShortHandReturn = styled.div.attrs(() => ({ }))`
  /* styled here are still colored correctly */
  color: red;
  background-color: white;
  padding: ${(p) => p.color};
`;

const StyledButton = styled(({ color1, ...other }) => (
  <Button {...other} classes={{ label: 'label' }} />
))`
  border: 0;
  color: white;
`;

const Root = css<RootProps>`
  height: ${props => (props.label ? 72 : 48)}px;
`;

The third root is not coloured properly. I think because my new addition overrides the meta.arrow.tsx syntax and collides.

jasonwilliams added a commit to jasonwilliams/vscode-styled-components that referenced this issue May 28, 2021
@jasonwilliams jasonwilliams linked a pull request May 28, 2021 that will close this issue
@jasonwilliams
Copy link
Collaborator

@bluenex if you wanted to fork my PR its here:
#300

@panda919
Copy link
Author

panda919 commented Jul 2, 2021

Sorry for my late reply.
So what is the solution?

@MultidimensionalLife
Copy link

Same issue here. Any updates folks? 😊

@jasonwilliams
Copy link
Collaborator

jasonwilliams commented Aug 10, 2021

There's no update here, my PR fixes one thing but breaks another. This project doesn't have many contributors so unless you're willing to pitch-in it will just be a case of waiting for someone to find a way through. There's 34 other issues that need attention so I'm unable to dedicate a lot of time to this.

@gpoole
Copy link

gpoole commented May 18, 2022

Wow that's a complicated regex!

I've found this issue crops up now and then for me and in the past I've just ignored it or stopped using attrs and created a wrapper component, which isn't ideal. Thanks to the comments above it seems like a still fairly convenient and readable workaround is just naming the attrs mapping function:

const buttonAttrs = ({ primary }) => ({
  type: primary ? "submit" : "button"
});

const Button = styled.button.attrs(buttonAttrs)`

`;

@nfour
Copy link

nfour commented Jul 13, 2022

Figured out a work around:

image

const $Text = styled((p: PropsOf<typeof TextField>) => (
  <TextField
    variant="outlined"
    multiline
    placeholder="..."
    InputLabelProps={{ shrink: true }}
    {...p}
  />
))(
  () => css`
    flex-grow: 1;
    margin-top: 1em;
  `
)

Highlighting remains good

@iwan-uschka
Copy link

iwan-uschka commented Sep 6, 2022

I played around a little and found another workaround by adding

// `

after a styled component definition like below.

with
without

@iwan-uschka
Copy link

We just decided to avoid using attrs at all because it causes warnings like this:

Warning: Prop `className` did not match. Server: "sc-hAQmFe fwszIZ" Client: "sc-dkrFOg evlvFY"

After removing/refactoring attrs the warnings disappear. We had some serious trouble with differences between SSR and CSR.

@Martinocom
Copy link

Still present today and we (as company) cannot decide to remove attrs, unfortunately.

VS Code 1.83.1
Mac Book Pro with M2, 14inch 2023, OS Ventura 13.4.1
vscode-styled-components v1.7.8

Not working

If using () => { } syntax on single or multi-line.

Screenshot 2023-10-16 at 09 57 35

Working 1

If using () => { } as single line or...

Screenshot 2023-10-16 at 09 57 59

Working 2

If using { } syntax, not the () => { } one.

Screenshot 2023-10-16 at 10 08 52

Extra notes

I'm using Palenight Theme and Material Icon Theme for styling my VsCode. I have also Color Highlight and ESLint plugins.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants