Skip to content
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

Allow this in property and call descendants in render assignments #213

Closed

Conversation

bearfriend
Copy link

@bearfriend bearfriend commented Sep 10, 2024

A ThisExpression that is the descendant of a property or call within an assignment should not trigger assignmentFound

@bearfriend bearfriend changed the title Allow descendant ThisExpression on left-hand side Allow descendant ThisExpression on render left-hand side Sep 10, 2024
@bearfriend bearfriend changed the title Allow descendant ThisExpression on render left-hand side AllowThisExpression property descendants on render left-hand side Sep 10, 2024
@bearfriend bearfriend changed the title AllowThisExpression property descendants on render left-hand side Allow ThisExpression property descendants on render left-hand side Sep 10, 2024
@bearfriend bearfriend changed the title Allow ThisExpression property descendants on render left-hand side Allow this in property and call descendants in render assignments Sep 10, 2024
@43081j
Copy link
Owner

43081j commented Sep 11, 2024

Are you trying to catch these two cases?

this.foo.bar = 123;

fn(this).bar = 123;

I feel like the pattern will be too specific right now. It'll probably be easier to maintain if we keep the same pattern but update the rule logic

Also, both of these mean you're mutating something in your render method, which is what this rule is meant to prevent

Could you give a real world example of why someone might need this?

@dlockhart
Copy link

Could you give a real world example of why someone might need this?

From our recent lock file update to get these changes (which because of the better Lit element detection in the latest release is now finally applying these rules to some of our elements):

This failed:

render() {
  const secondaryPanelStyles = {};
  if (this._isResizable()) {
    secondaryPanelStyles[this._isMobile ? 'height' : 'width'] = `${size}px`;
  }
}

And had to become:

render() {
  const secondaryPanelStyles = {};
  if (this._isResizable()) {
    const dimension = this._isMobile ? 'height' : 'width';
    secondaryPanelStyles[dimension] = `${size}px`;
  }
}

So no mutation of anything on "this" in the render.

@bearfriend
Copy link
Author

bearfriend commented Sep 11, 2024

@dlockhart laid out our real world example of the .property descendant case (thanks!). The CallExpression case is as you guessed:

fn(this).bar = 123;

This mutates something but it doesn't mutate this.

I'm not terribly familiar with eslint plugins but I can take a stab at converting my selector changes to rule logic, though I would think it's pretty stable as it seems like these are the only two ways you can get a ThisExpression as a descendant of an AssignmentExpression that shouldn't error.

@bearfriend
Copy link
Author

Closing in favor of #214

@bearfriend bearfriend closed this Sep 18, 2024
@bearfriend bearfriend deleted the no-this-assign-in-render-left-child branch September 18, 2024 16:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants