eslint-plugin-ember icon indicating copy to clipboard operation
eslint-plugin-ember copied to clipboard

[BUG] require-computed-macros can autofix to self-referential macros

Open runspired opened this issue 3 years ago • 3 comments

Given the following (admittedly incorrect) computed usage

storeIds: computed("storeIds", "line", function() {
        return this.storeIds;
    }),

This rule will convert it to the following, which blows up since the self-referential nature becomes more evident when an alias is installed under the hood.

storeIds: computed.reads('storeIds')

A better conversion would be

storeIds: null

runspired avatar Mar 27 '21 23:03 runspired

I wonder if it's worth creating a separate rule no-self-referencing-computed-properties?

bmish avatar Mar 27 '21 23:03 bmish

@bmish for the first example, absolutely. But since an existing rule also modifies that first example we'd have to make sure they didn't interfere with each other. It's also unlikely you end up with a self-referential computed using a macro without using autofix since it results in application errors almost immediately. Running this on a large app turned out to be problematic since now I'm chasing down self-referentials. Trying to decide whether to patch this rule to convert them better or if I submit a new rule.

runspired avatar Mar 28 '21 01:03 runspired

I actually think the new rule no-self-referencing-computed-properties should catch both macro and non-macro computed properties. No reason to limit it to just one computed property form. This new rule would serve to make it clear that something is likely seriously wrong with the self-referencing computed property in a way that requires user attention.

If we make any changes to require-computed-macros, I would just remove the autofix for this situation. I would not consider it acceptable to autofix to storeIds: null as that's a behavior change and hides the fact that something is broken and needs manual attention.

bmish avatar Mar 28 '21 15:03 bmish