Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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: add
lwc:on
directive #5228base: master
Are you sure you want to change the base?
feat: add
lwc:on
directive #5228Changes from 32 commits
af5ec53
43645d6
e448481
d80984f
aec5dc7
96df273
154a932
69bb3c2
d6a0a6a
80b6a07
c665ac0
c6721f9
52516ee
e036dd9
3b7d35a
35723ab
8a26014
e4a492d
e2362b8
46ebc04
785100a
eac5f81
e911556
ef2b742
884037c
2ee9351
56eed25
82cd9c8
efa9a5a
528f63a
c4cdbd5
7603842
724f925
cb1645c
d2f2ce8
2535c10
88e9a0f
abcce1b
021a922
247e660
528b1ca
1a5236b
2e5bf3d
5a2358b
9596f94
e5a2cda
523ac07
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 feel like, historically, we've made things like this just console warnings that only throw in dev mode. I also feel like, currently, we've decided that's an annoying pattern that nobody pays attention to, so maybe we should just go with this. @divmain, thoughts?
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.
Yes, we have typically used
logError
for this. But in this case, I think I prefer throwing an error - as you say, nobody pays attention to the warnings. And if we don't throw, somebody will eventually do the wrong thing and expect us to make it work.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 didn't notice earlier that
logError
is used elsewhere, In that case I would want to change this tologError
since we would want that (reactivity with same object, modified) to be available if need has been demonstrated.Also, that case (same object passed to
lwc:on
) works just as fine, if we don't throw, so there is no cost of 'making it work'.@divmain , is it fine if I make the change?
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.
Yes feel free.
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.
done