Skip to content
This repository has been archived by the owner on Aug 31, 2023. It is now read-only.

feat(rome_js_analyze): noParameterProperties #3874

Merged
merged 1 commit into from
Feb 13, 2023
Merged

feat(rome_js_analyze): noParameterProperties #3874

merged 1 commit into from
Feb 13, 2023

Conversation

Conaclos
Copy link
Contributor

Summary

This implements ESLint's no-parameter-properties.

Test Plan

Unit tests and doc-tests included.

@netlify
Copy link

netlify bot commented Nov 27, 2022

Deploy Preview for docs-rometools ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 14d9ebe
🔍 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.

@ematipico
Copy link
Contributor

The documentation says that this rule is deprecated

@Conaclos
Copy link
Contributor Author

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
Copy link
Contributor Author

Conaclos commented Nov 27, 2022

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.

@ematipico
Copy link
Contributor

ematipico commented Nov 27, 2022

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?

@Conaclos
Copy link
Contributor Author

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 #).

@leops
Copy link
Contributor

leops commented Nov 28, 2022

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

@Conaclos Conaclos changed the title feat(rome_js_analyze): noParameterProperties feat(rome_js_analyze): noParameterProperties Nov 28, 2022
@github-actions
Copy link

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

@xunilrj
Copy link
Contributor

xunilrj commented Dec 12, 2022

@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.

@Conaclos
Copy link
Contributor Author

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

@github-actions github-actions bot removed the S-Stale label Dec 13, 2022
@Conaclos
Copy link
Contributor Author

Conaclos commented Jan 4, 2023

@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 Conaclos requested review from ematipico and removed request for xunilrj and leops January 5, 2023 18:07
@Conaclos Conaclos added this pull request to the merge queue Feb 13, 2023
Merged via the queue into rome:main with commit 1085ecc Feb 13, 2023
@Conaclos Conaclos deleted the no_parameter_properties branch February 13, 2023 21:37
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants