-
Notifications
You must be signed in to change notification settings - Fork 32
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
feat: update rule no-unsupported-ssr-properties
to reflect SSR best practices @W-14387292
#133
Conversation
522c87c
to
69631b0
Compare
There was a problem hiding this 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(); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?.()
.
69631b0
to
d5059bc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
No description provided.