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

feat: update rule no-unsupported-ssr-properties to reflect SSR best practices @W-14387292 #133

Conversation

seckardt
Copy link
Contributor

No description provided.

@seckardt seckardt force-pushed the feature/248-W-14387292-update_no-unsupported-ssr-properties branch 7 times, most recently from 522c87c to 69631b0 Compare October 31, 2023 17:07
Copy link
Contributor

@divmain divmain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally LGTM. I did have a couple of questions, and I'd like an answer to the optional call expression nuance.

@@ -29,6 +29,42 @@ Examples of **correct** code for this rule:
```js
import { LightningElement } from 'lwc';

export default class Foo extends LightningElement {
connectedCallback() {
this.querySelector?.('span')?.foo();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would have thought this would need to be this.querySelector?.('span')?.foo?.(). But it seems that the optional chaining operator right before the parenthesis are not always required. The language rules around this are a little opaque to me, and I'd like to ensure we're pointing folks in the right direction with this guidance.

Here's a demonstration of what I'm talking about:

> const a = {}
undefined
> a?.foo?.bar()
undefined
> a?.foo?.bar?.()
undefined
> a.foo = {};
{}
> a?.foo?.bar()
Uncaught TypeError: a?.foo?.bar is not a function
> a?.foo?.bar?.()
undefined

That TypeError is concerning. It seems like if this.querySelector?.('span') returns an element, but that element doesn't have a foo function, this.querySelector?.('span')?.foo() will cause a TypeError to be thrown, which is probably not what we want.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@divmain Yeah, looking at the runtime code you pointed me to, it seems like the well-intentioned, but problematic "polyfill" behavior in the runtime implements/provides methods like querySelector, yet lets them throw an error when called?! That's pretty unfortunate, as due to that behavior the linting rule to catch all posible issues in that area will have to be much more complex and needs to be more contextual to cover forcing the whole statement to either be guarded or fully be made optional, e.g. a?.foo?.bar?.().

lib/rules/no-unsupported-ssr-properties.js Show resolved Hide resolved
lib/rule-helpers.js Outdated Show resolved Hide resolved
@seckardt seckardt force-pushed the feature/248-W-14387292-update_no-unsupported-ssr-properties branch from 69631b0 to d5059bc Compare November 1, 2023 10:47
Copy link
Contributor

@divmain divmain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

@divmain divmain merged commit fd29cd6 into salesforce:master Nov 2, 2023
6 checks passed
@seckardt seckardt deleted the feature/248-W-14387292-update_no-unsupported-ssr-properties branch December 15, 2023 07:53
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.

None yet

2 participants