Add new no-get-properties rule

[bmish]

No description provided.

[Turbo87]

can you add a test case for e.g. this.getProperties('while')? this should not convert to let { while } = ... since that would be illegal. but then again, this rule only triggers when there was already destructuring used before, right?

(see also https://github.com/rondale-sc/es5-getter-ember-codemod/blob/6e7929832301485ff15c0f4384363a1040eabb5c/transforms/es5-getter-ember-codemod/index.js#L3-L7)

[bmish]

Interesting! However, I don't think that applies here, because this rule is only suggesting removing the getProperties call, and not changing the existing destructuring.

[rwjblue]

FWIW, applying this rule across the board is not "safe".

Specifically, destructuring from any object that is an Ember.ObjectProxy, an Ember.ArrayProxy, or any object with an unknownProperty method will result in an error. The documentation for the new rule should be updated to include this information.

I believe the rule should only apply to this.getProperties and getProperties(this, ...) by default (calling someForeignObject.getProperties is much more problematic due to the exceptions I mentioned above).

[bmish]

@rwjblue thanks for the reminder about proxies! I removed the autofixer as a result. I also updated the documentation to call that out.

However, I would prefer if the rule continues to catch someForeignObject.getProperties, because:

• It no longer autofixes
• es5-getter-ember-codemod currently converts that (the codemod isn't limited to this only)

How does that sound?

[Turbo87]

I would propose to keep the autofixer, but make the rule configurable. By default it would only work for this, but you could either configure other variable names for which it should also work, or you define a wildcard or something like that 🤔

[rwjblue]

es5-getter-ember-codemod currently converts that (the codemod isn't limited to this only)

There are a number of reported issues on that repo around this exact issue. The codemod absolutely should be restricted to this.

[bmish]

@rwjblue thanks for calling that out, I updated this rule to be limited to this.getProperties only.

@Turbo87 I would still lean towards leaving the autofixer out because:

• If someone wants to fix violations, they can run the codemod.
• Even when the rule is limited to this.getProperties only, it still can easily cause breaking behavior changes because of proxy objects / unknownProperty.

However, I'd like to follow-up in a separate PR to add an catchForeignObjects boolean configuration property (default false) to this rule for those who want to be more aggressive.

[Turbo87]

👍