salesforce / eslint-plugin-lwc

Official ESLint rules for LWC
MIT License
98 stars 32 forks source link

consider Improving no-api-reassignments caveats #91

Open tomekduda opened 2 years ago

tomekduda commented 2 years ago

Evening! I'm slapped with "no-api-reassignment" violation on an lwc variable that yes, is exposed as @api but only because I need the values in a flow. In meta.xml file I have

<targets>
    <target>lightning__FlowScreen</target>
    </targets>
<targetConfigs>
    <targetConfig targets="lightning__FlowScreen">
        <property role="outputOnly" name="values" type="String[]" description="Whatever" />
    </targetConfig>
</targetConfigs>

I mean I know that for normal parent-child communication I'm supposed to use events, cool. But what about flows? What's "better"? Disable the eslint rule or convert the variable to getter? If this is an acceptable limitation (I don't expect js file's parser to jump over to meta.xml and have a look...) - can you add flows to the list of caveats? If getter is more appropriate - can you write that as alternative example of correct code?

pmdartus commented 2 years ago

I am not familiar with how flow works, so bear with me. As far as I understand the meta XML file, your component looks like this:

import { LightningElement } from 'lwc';

export default class Flow extends LightningElement {
    @api values;

    handleSomeEvent() {
        this.values = /* some random value */;
    }
}

In which case, the node-api-reassignment linting rule reports this.values as an invalid API reassignment. Correct? If it's the case then, I would recommend using a pair of getter/setter to expose the updated value to the flow.

tomekduda commented 2 years ago

I avoid flows since they had that Flash-based editor so I'm in same boat. Yes, this looks close to my use case.

I've read some documentation and apparently there's an event I should be raising on change (https://developer.salesforce.com/docs/component-library/documentation/en/lwc/lwc.use_flow_runtime_considerations), good to know.

And example that uses private properties + @api getter setter. I'd say this is bit hard to come across if all you need is to emit data out of LWC and you don't care about previous/next buttons...

So yeah, would you consider adding an example similar to this or even just the link in the docs/rules/no-api-reassignments.md ?

tehnrd commented 2 years ago

I was literally about to open an issue for this exact issue! I'm pretty sure this warning can be ignored in the context of an LWC component that is built specifically for a Flow.

joeythomaschaske commented 1 year ago

@pmdartus what is the correct way to use a getter/setter for this? A discussion on a PR came up today addressing this issue and we can't find the correct answer.

Our first approach was this

import { LightningElement, api } from "lwc";
export default class TodoItem extends LightningElement {
  _itemName;

  @api
  get itemName() {
    return this._itemName;
  }

  set itemName(value) {
    this._itemName = value;
  }

  setName(evt) {
    this._itemName = evt.target.value;
  }
}

In our PR reviews it was suggested that the @api contract expects the getter value to always be the same as the original value passed to the setter.

Using getters and setters ensures that the public API contract is easily enforced. Don’t change the value of a property that’s annotated with @api.

Honoring the above, should it actually look like this?

import { LightningElement, api } from "lwc";
export default class TodoItem extends LightningElement {
  _itemName;
  _originalName;

  @api
  get itemName() {
    return this._originalName;
  }

  set itemName(value) {
    this._originalName = value;
    this._itemName = value;
  }

  setName(evt) {
    this._itemName = evt.target.value;
  }
}