Fix inverted validate label check in NcInputField #3980
Conversation
Signed-off-by: Raimund Schlüßler <[email protected]>
raimund-schluessler
added
bug
raimund-schluessler
requested review from
susnux,
skjnldsv,
Pytal,
ShGKme and
marcoambrosini
Signed-off-by: Raimund Schlüßler <[email protected]>
Signed-off-by: Raimund Schlüßler <[email protected]>
Signed-off-by: Raimund Schlüßler <[email protected]>
| if (!isValidLabel) { | ||
| console.warn('You need to add a label to the NcInputField component. Either use the prop label or use an external one, as per the example in the documentation.') | ||
| } |
There was a problem hiding this comment.
Warn in computed = log every time the computed changes. It might not be the desired outcome
I'm not sure this is a good idea 🤔
There was a problem hiding this comment.
Wouldn't the same happen if we warn in the watcher? The computed changes, the watcher fires, checks the label and warns in case it is not valid!?
There was a problem hiding this comment.
Hum, good point 🙈
I'm weirded out by this anti pattern though 😁
There was a problem hiding this comment.
Are we sure the computed is not executed on other occasions? if so, you can merge !
There was a problem hiding this comment.
The computed is triggered every time label or labelOutside is changed, but of cause the warning will only be fired if one of both is falsy.
There was a problem hiding this comment.
@skjnldsv I don't know whether it's an anti-pattern, but we do the same here for example: https://github.com/nextcloud/nextcloud-vue/blob/master/src/mixins/actionText.js#L87-L93 🙈
The computed is triggered every time
labelorlabelOutsideis changed, but of cause the warning will only be fired if one of both is falsy.
I would think so too. If label changes between False, undefined, null, an empty string or anything else evaluating to False, the warning would be triggered again even though it was invalid before already. But that seems a bit theoretic to me.
There was a problem hiding this comment.
Let's hear what @ShGKme thinks about this approach 🙂
There was a problem hiding this comment.
@skjnldsv I don't know whether it's an anti-pattern, but we do the samer here for example:
master/src/mixins/actionText.js#L87-L93 see_no_evil
looks at blame
sees @skjnldsv
closes tab
There was a problem hiding this comment.
Let's hear what @ShGKme thinks about this approach 🙂
It's too late, but yes 😅.
It proper works, but it is an anti-pattern, computed should never have side-effects, only computing the value.
In options API there is a place for computed + watch, in Composition - watchEffect.
Pytal
added
4. to release
The
NcInputFieldcomponent currently warns about a missing label only if a label is provided and if the label changes during the lifetime of the component (which probably is the reason why no one noticed this yet). This PR fixes the inverted logic and runs the check on component creation as well. This might spawn quite a few warnings in the apps using this component, but I guess that's what this warning is for 🤷As a side note, I would propose to change from
throw new ErrortoVue.warn, but that's a matter of taste, I guess.