Skip to content

'Strict' option for vue/no-unused-properties #1636

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

Closed
dsl101 opened this issue Sep 24, 2021 · 11 comments
Closed

'Strict' option for vue/no-unused-properties #1636

dsl101 opened this issue Sep 24, 2021 · 11 comments

Comments

@dsl101
Copy link
Contributor

dsl101 commented Sep 24, 2021

What rule do you want to change?
vue/no-unused-properties

Does this change cause the rule to produce more or fewer warnings?
More when strict: true

How will the change be implemented? (New option, new default behavior, etc.)?
I have argued that this should be the default behaviour, in line with externally-called methods being considered 'unused' unless explicitly labelled in JSDoc style. But, if this change is undesirable, a strict: true option to vue/no-unused-properties might be a compromise, with the possibility of making strict: true the default in future.

Please provide some example code that this change will affect:

<script>
  export default {
    data () {
      return {
        unusedData: 42
      }
    },
    computed: {
      notReferenced () {
        return 'never seen'
      },
    },
    mounted () {
      const handlerName = 'eventHandler'
      // Event registration code ommitted, but would end up calling via:
      this[handlerName]
    },
    methods: {
      eventHandler () {
        // Used
      },
      notused () {
        // should show as warning
      }
    }
  }
</script>

What does the rule currently do for this code?
The presence of this[handlerName] (as noted in the original ticket) makes it almost impossible for static analysis to determine whether any particular method or property is actually used. The current behaviour appears to assume that all properties, methods, data, and computeds are therefore used, and the above code produces no warnings at all. Commenting out this[handlerName] restores the (in my opinion, correct) behaviour that only explicitly referenced properties are 'used'.

What will the rule do after it's changed?
Warn about any properties not explicitly referenced, or which are labelled as referenced by the developer using either // eslint-disable-next-line vue/no-unused-properties or /** @public */ annotations.

@ota-meshi
Copy link
Member

I don't like the option to change the way it works, but I don't reject to add the option to change the way it works.

However, the name of the option can be confusing.
I'd like to have an option name that makes it easy to understand how to handle properties that can't be tracked.

@dsl101
Copy link
Contributor Author

dsl101 commented Sep 24, 2021

I'm not precious about the name of the option—strict came to mind simply because of the analogy with JS use strict and the fact it turns on a more strict definition of 'unused'.

explicit is an alternative? When true, only explicitly referenced properties are considered used? Did you have something in mind that's less confusing?

@ota-meshi
Copy link
Member

There are some patterns that this rule considers to have used all properties.

e.g.

<script>
  /* eslint vue/no-unused-properties: [2, {groups:['data']}] */
  export default {
    data () {
      return {
        unusedData: 42
      }
    },
    methods: {
      fn () {
        return this
      }
    }
  }
</script>

I think we need to allow the user to set whether to consider each as using all properties or not.

The option we add is an option that controls the decision to access an unknown property, so I want the option name to be descriptive.
But I can't think of a good name 😓.

@dsl101
Copy link
Contributor Author

dsl101 commented Sep 24, 2021

I'm not sure I completely follow what you were trying to illustrate there—did you mean this new setting should be per group? If so, how about assume-unused: ['data'] to indicate that any data element not explicitly referenced should be assumed to be unused.

@dsl101
Copy link
Contributor Author

dsl101 commented Sep 24, 2021

Although, I'm now not seeing the point of including 'data' in the groups: setting—and I'm back to why this isn't really just a bug. If I don't want to be warned about unused props, data, etc., I would just leave them out of the groups: set. Maybe I misunderstood what your last post was meaning...

@ota-meshi
Copy link
Member

ota-meshi commented Sep 24, 2021

I wanted to say that the last example I wrote would not be warned by return this. If you comment out return this, unusedData will be warned.

You can try it DEMO.

image

@ota-meshi
Copy link
Member

For example, if the option name was strict, it would be confusing to the user how to handle return this.

@dsl101
Copy link
Contributor Author

dsl101 commented Sep 24, 2021

When you say ”how to handle return this“, what are you expecting them to do?

I still argue that warning about unused properties according to the existing settings should not be affected by the code containing a line like return this, and thus this really is a bug. But if you’re still keen that this current behaviour is correct, then what I’m after is a stricter interpretation of ‘unused’. I’m not sure what’s confusing about that as an option name.

@ota-meshi
Copy link
Member

I was misunderstanding your option suggestions, but I probably understood what you were saying.
However, I don't agree with naming the option strict.
Strictly speaking, all reports for this rule are false positives as all properties are accessible via $refs etc. (for Vue<3.2 or without expose).
So I think it hard to understand what strict means.

The current behavior is implemented to be false negatives to avoid false positives as much as possible when the property usage becomes untraceable. I think it's a user's preference how to handle some of these processes, so I think it should be possible to set each one.

For example, how about adding the following option to control known processes?

{
  "vue/no-unused-properties": ["error", {
    "stopReporting": [ // To avoid false positives, stop reporting if the specified process is found.
      "unknownPropertyAccess", // e.g. this[unknown]
      "returnInstance", // e.g. return this
      // There may be other than these.
    ]
  }]
}

I think the settings you want are:

{
  "vue/no-unused-properties": ["error", {
    "stopReporting": [] // Probably "strict" for you as it doesn't stop the reporting.
  }]
}

What do you think? (I'm not good at English, so option names can still be confusing to people 😓)

@ota-meshi
Copy link
Member

If anyone wants to work on this change, open a PR.
If no one works for a long time, I will close this issue.

@dsl101
Copy link
Contributor Author

dsl101 commented Sep 29, 2021

Right, so there would be a default value of stopReporting which would match the current behaviour, and by setting that to an empty array, it would not stop reporting for any ambiguous (e.g. this[handlerName]) reference? I think I follow, and that sounds like it would do what I'm after.

I don't think I know the internals well enough to start on this, so hopefully some kind soul will jump in :).

dsl101 added a commit to dsl101/eslint-plugin-vue that referenced this issue Mar 8, 2023
Ref vuejs#1636, this is my first stab at adding an option to not treat generic access to `this` as 'using' properties / data / computed, etc. If this is the right approach, I can try adding tests if needed. It's more general I think than what you'd proposed in that issue, but for my use case it works well.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants