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

Counting polymorphic prop / abstractions #24

Open
douglaseggleton opened this issue Dec 5, 2020 · 4 comments
Open

Counting polymorphic prop / abstractions #24

douglaseggleton opened this issue Dec 5, 2020 · 4 comments

Comments

@douglaseggleton
Copy link
Contributor

Hey 👋🏻 Recently took a lot of inspiration from this on a project I worked on recently. Great tool! :D

Two of the common issues I came across was using styled components as={MyComponent} and components such as const CustomButton = ({...rest}) => <Button {...rest}/>.

We ultimately decided/accepted we couldn't accurately count every instance with static analysis, we were more interested in a general pattern of adoption and rephrased to "this component is used at least .... many times".

Just wanted to know if you had any thoughts/ideas on this? 🤔

@moroshko
Copy link
Owner

moroshko commented Dec 9, 2020

Hey @douglaseggleton,

I can't immediately see what would stop us from counting as={MyComponent} instances. The AST for <Button as={MyButton}>Submit</Button> contains "MyButton" as a string, so curious to know why you found this difficult.

Regarding const CustomButton = ({...rest}) => <Button {...rest}/> - what exactly would you like to count?

Could you elaborate a bit more on what you were trying to achieve, and what was the challenge?

@ajkl2533
Copy link

ajkl2533 commented Sep 21, 2021

Hi @moroshko I would like to iterate on this issue as I also hit this issue with styled-components. It seems that referenced components are not reported.
See this example:

import styled from 'styled-components';
import {Text} from 'design-system';

const WrappedText = styled(Text)`
  ...
`

const Test = () => (
  <>
    <Text variant="secondary">Secondary</Text> // <-- this gets reported
    <p as={Text}>Polymorfic</p> // <-- this is not reported as Text
    <WrappedText variant="danger">Wrapped</WrappedText>  // <-- this is not reported as Text
  </>
)

and the raw output for this is

{
  "Text": {
    "instances": [
      {
        "importInfo": {
          "imported": "Text",
          "local": "Text",
          "moduleName": "design-system",
          "importType": "ImportSpecifier"
        },
        "props": {
          "variant": "secondary"
        },
        "propsSpread": false,
        "location": {
          "file": ".../react-scanner-test/src/index.jsx",
          "start": {
            "line": 10,
            "column": 5
          }
        }
      }
    ]
  }
}

I also was checking the original AST from where you are compiling the output and yes the info is there so it is probably just a matter of extending parsing AST to catch also those cases.

BTW this is an awesome package and if this polymorphic props issue will be solved it could really help lots of teams to do sensible measurements on their project 🙏

EDIT:
I'm pasting here also the original AST for the example above https://pastebin.com/vfYa32X8

@jenreiher-eb
Copy link

I also ran across a situation like

 <OtherComponent
   prop={Text} />

was not being captured by the configuration. After some digging, I found that it's because the correct match for this is JSXExpressionContainer not JSXOpeningElement. I've done some hackery/code duplication to get it working locally at least for my use case...
but as an ex-engineer (now PM) I recognize that there's a lot of room for improvement here and y'all probably don't want that cruft in this lovely library.

Would be happy to share my chicken scratch though if anyone wanted to take it an run with it as a proper open source update?

@smol-honk
Copy link

May not be what you're looking for, but I added a PR for checking if there's a styled component #56

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

No branches or pull requests

5 participants