tools icon indicating copy to clipboard operation
tools copied to clipboard

feat(rome_js_analyze): `noParameterProperties`

Open Conaclos opened this issue 3 years ago • 7 comments

Summary

This implements ESLint's no-parameter-properties.

Test Plan

Unit tests and doc-tests included.

Conaclos avatar Nov 27 '22 14:11 Conaclos

Deploy Preview for docs-rometools ready!

Built without sensitive environment variables

Name Link
Latest commit 14d9ebec55a80df691de6a6bc09137f911482889
Latest deploy log https://app.netlify.com/sites/docs-rometools/deploys/63eaa9afff494b00084858f6
Deploy Preview https://deploy-preview-3874--docs-rometools.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

netlify[bot] avatar Nov 27 '22 14:11 netlify[bot]

The documentation says that this rule is deprecated

ematipico avatar Nov 27 '22 16:11 ematipico

The documentation says that this rule is deprecated

Yes, the rule was replaced by parameter-properties. However, this does not fit with the philosophy of Rome (no or use, and do not provide configuration when it is not necessary).

Conaclos avatar Nov 27 '22 16:11 Conaclos

By the way, I tried to implement an action to turn the parameter property into a regular property.

However, I encountered an issue: when I mutate a node, I cannot mutate a descendant without invalidating the previous mutation. It is particularly inconvenient.

I have to figure out how to overpass this limitation.

Conaclos avatar Nov 27 '22 17:11 Conaclos

I have some reservations about this rule, here's my concerns:

  • if and once this rule is promoted, which group would this rule fit?
  • there are frameworks that use these properties, such as NestJS: https://docs.nestjs.com/providers#services, wouldn't this rule create friction?
  • are there any other motivations other than the the confusing part? I used parameter properties inside constructors many times, and I fail to see this rule in the Rome grand scheme. Would you help me to understand this rule?

ematipico avatar Nov 27 '22 20:11 ematipico

if and once this rule is promoted, which group would this rule fit?

I think that Style could be a good fit.

there are frameworks that use these properties, such as NestJS

I was not aware of this use. It could definitively bring friction.

are there any other motivations other than the the confusing part?

The arguments I could add:

  • TypeScript should avoid code generation. Parameter properties are syntactic sugars. Yes, TypeScript has other features that generate code (enums, namespaces). However, namespaces are no longer recommended, and enums are popular and could even be one day part of JavaScript.
  • It is not possible to declare a private parameter property (using she-bang #).

Conaclos avatar Nov 27 '22 21:11 Conaclos

By the way, I tried to implement an action to turn the parameter property into a regular property.

However, I encountered an issue: when I mutate a node, I cannot mutate a descendant without invalidating the previous mutation. It is particularly inconvenient.

I have to figure out how to overpass this limitation.

I think the easiest way to implement this without encountering conflicts in the mutation would be to:

  • Remove the TsPropertyParameter from its parent JsConstructorParameterList using splice_slots
  • Replace the old constructor parameter list with the modified list in the JsClassMemberList containing the constructor using replace_child
  • Insert the new JsPropertyClassMember node in the modified class member list using splice_slots
  • Queue an operation to replace the old class member list with the modified list in the BatchMutation by calling replace_node

leops avatar Nov 28 '22 09:11 leops

This PR is stale because it has been open 14 days with no activity.

github-actions[bot] avatar Dec 12 '22 12:12 github-actions[bot]

@Conaclos Do you have a version with the mutation problem so I can look it up? I think we may have a version of BatchMutation that fix this, but it was never merged because it is slower.

xunilrj avatar Dec 12 '22 13:12 xunilrj

@xunilrj I committed my last wip commit. I did not take a look to your proposal yet.

Conaclos avatar Dec 12 '22 16:12 Conaclos

@xunilrj I think that the action could be made in a separate PR. I could like to propose the PR in its current state.

Conaclos avatar Jan 04 '23 18:01 Conaclos