Adding support for custom prop values

andrewmtam commented 3 years ago

Awesome tool you've built out here! One additional use case I've had for this scanner is to see what kinds of values are passed for props across the codebase for a specific component.

While your program handles most of the straightforward cases, it does not handle the case for when more complex properties are passed in as props.

If you're open to changing this behavior, I have a couple ideas!

Approach 1 -- Adding `getPropValue` to the configuration file

This PR covers the approach of making getPropValue configurable, so that consumers can format the props however they want, instead of using the defaults (which is great when the prop value is a Literal, but not as convenient for any other data type).

Here's what my config file looks like to get this working:

const escodegen = require("escodegen-wallaby");

module.exports = {
  getPropValue: (node) => {
    if (node.type === "JSXExpressionContainer") {
      return escodegen.generate(node.expression);
    } else {
      return escodegen.generate(node);
    }
  },
};

This tool escodegen-wallaby is very cool in that it takes an AST node, and actually converts it back to the literal code for that node! Also, I had to use escodegen-wallaby instead of escodegen because escodegen does not support JSX type nodes, where as escodegen-wallaby does

Approach 2 -- Changing `getPropValue` in `scan.js` to use `escodegen-wallaby`

For this approach, things would be more automated. If we just changed the core method:

https://github.com/andrewmtam/react-scanner/blob/75eb048c6315da5131d3a6fbc3c4cee1bbbbb14c/src/scan.js#L30-L49

to use escodegen-wallaby altogether, everyone would get this behavior out of the box!

Approach 3 -- Add a new configuration parameter, `showDetailedProps`

This is kind of a combination of Approach 1 and 2. If we want to keep getPropValue the way it is, for preserving backwards compatibility, but still like the idea of reporting the prop's full value, we can use a flag that switches between these methods.

Examples of new `prop` values

Using escodegen-wallaby, here's some examples of what could now be reported for props:

        "props": {
          "onClick": "() => {\n    if (someFunction) {\n        thenCallSomeFunction();\n    }\n    test.push(blah);\n}"
        },
...
        "props": {
          "loading": "loading",
          "onClick": "thisIsAVariableName"
        },
...
        "props": {
          "activeSortKey": "this.state.localReactState",
          "activeSortType": "this.state.moreLocalReactState",
          "sortKey": "key",
          "onClick": "sortKey => this.setSort(sortKey, sortFunc)"
        },

Let me know what you think!

moroshko commented 3 years ago

Thanks @andrewmtam for this!

Let's go with approach 1 which you implemented.

Some things that I'd change:

Let getPropValue to accept component and prop names as well so that users could do "include expressions only for prop X in component Y). For example: getPropValue({ value, componentName, propName }). My gut feel that componentName should be the same as here.
Add a new "Customizing prop values treatment" section in the README, describe the signature of getPropValue so people could see what parameters are available, and provide a real-world example to demonstrate how it works and what the output is. Link to this section from the Config file table where you mention getPropValue for the first time.
Add this same example as a test. This way our tests are a bit more realistic and we ensure that what we say in the README works as expected.

Thanks again for your contribution ❤️

andrewmtam commented 3 years ago

Sweet, thanks for the feedback! Updated the PR to reflect the requested changes; let me know if you'd like me to change anything else

moroshko commented 3 years ago

@andrewmtam Thanks a lot for this contribution! I released 1.0.0 which contains this change.

andrewmtam commented 3 years ago

Np, thanks for the guidance and helping merge this in so quickly!

moroshko / react-scanner