Skip to content

Comments

missing element warnings#217

Merged
leastbad merged 1 commit intomasterfrom
element_warnings
Aug 26, 2022
Merged

missing element warnings#217
leastbad merged 1 commit intomasterfrom
element_warnings

Conversation

@leastbad
Copy link
Contributor

Here's a pass at customizable missing element warnings. I'm not attached to my approach; feel free to suggest different variable names, different attribute names, changes to the behaviour of the different modes.

I should also say that it occurred to me only as I was typing this up that perhaps this is actually the wrong approach; instead of specifically addressing missing elements, perhaps it would be better to implement a debug concept that would allow the developer to make decisions for all errors. There are lots of warnings etc in the codebase that have nothing to do with missing elements. Opinions, please!

After deliberation, I took Marco's idea to have multiple strategies and ran with it. I figure that we can always pull these extra options out if we don't want them.

CableReady.initialize now supports an onMissingElement option which sets a global value. As always, it defaults to warnings (warn) but also supports ignore, event, exception. It will warn if you pass an invalid option, falling back to warn.

CableReady.initialize({ consumer, onMissingElement: 'event' })

event raises a cable-ready:missing-element event on document. exception throws an exception.

The emitMissingElementWarnings option on perform and performAsync has been removed in favor of onMissingElement. It's a major version bump, after all. A value provided overrides the global value.

if (data.cableReady) perform(data.operations, { onMissingElement: 'exception' })

We could optionally test for emitMissingElementWarnings: false, issue a deprecation warning and set onMissingElement: 'ignore', but it's not implemented at this time. Opinions, please!

Finally, the stream_from web component checks for a missing attribute and falls back to the global setting. The WC supports warn, ignore and event, but not exception. This is because there's no way for a developer to catch an exception raised by the WC.

<%= stream_from :all_users, html_options: {missing: :ignore} %>

Again, I'm happy to change the attribute. I started with short and simple, because missing-element requires using double-quotes which I find ugly:

<%= stream_from :all_users, html_options: {"missing-element": :ignore} %>

@leastbad leastbad added enhancement proposal javascript Pull requests that update Javascript code labels Aug 22, 2022
@leastbad leastbad added this to the 5.0 milestone Aug 22, 2022
@leastbad leastbad requested review from hopsoft and marcoroth August 22, 2022 20:23
@leastbad leastbad self-assigned this Aug 22, 2022
Copy link
Contributor

@hopsoft hopsoft left a comment

Choose a reason for hiding this comment

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

I think we're ok to skip the deprecation warning as it's not a breaking change in the classic sense. Folks that use this feature may have some confusion, but like you noted... this is a major version bump.

I'm ok with using missing for the custom element, but more consistency with the JavaScript options might be nice. Not a hill I'd die on though.

LGTM

@leastbad leastbad merged commit 14e0559 into master Aug 26, 2022
@leastbad leastbad deleted the element_warnings branch August 26, 2022 07:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement javascript Pull requests that update Javascript code proposal

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants