More fine-grained parsing of data attributes

[bencroker]

Someone reported having a data attribute collision between a Craft plugin they use and Datastar. The plugin uses data-show-processing-spinner to mark elements as processing., which results in this error.

Uncaught (in promise) Error: Expression function not created

@delaneyj put aside any thoughts of “forms are bad”, because this is a plugin for collecting user input from the frontend. So it is less about forms and more about dealing with naming collisions.

Would it be possible to get rid of mustHaveEmptyKey and replace mustNotEmptyKey with mustHaveKey? That way, data-show-processing-spinner will not be confused with the data-show attribute.

[delaneyj]

I think this way lies madness. There's too many edge cases to do this properly probably.

If you don't want name space clashing

1. Modify the other library
2. Make a custom bundle with different names

Class.name = 'clazz'
Data.load(Class)

Now have data-clazz

[bencroker]

I’m going to push back on this. If the attribute plugin’s name is show, and it does not accept a key, then the plugin should ignore data-show-* attributes. I don’t see any madness here. On the contrary, this seems more logical than the current implementation, which errors out when it encounters data-show-processing-spinner. This may require a canHaveKey option, but either way, I think it merits exploring for 0.21.0.

[bencroker]

Proof-of-concept in engine.ts.

Before:

if (p.mustHaveEmptyKey && key.length > 0) {
    // must have empty key
    throw ERR_BAD_ARGS;
}
if (p.mustNotEmptyKey && key.length === 0) {
    // must have non-empty key
    throw ERR_BAD_ARGS;
}

After:

if (!p.canHaveKey && key.length > 0) {
    // skip
    continue;
}
if (p.mustHaveKey && key.length === 0) {
    // must have a key
    throw ERR_BAD_ARGS;
}

[delaneyj]

i still have a major problem with trying to maintain this separation... we may add modifiers in the future, we can "solve" this for now but it seems like a game of whack a mole

[delaneyj]

Proof-of-concept in engine.ts.

Before:

if (p.mustHaveEmptyKey && key.length > 0) {
    // must have empty key
    throw ERR_BAD_ARGS;
}
if (p.mustNotEmptyKey && key.length === 0) {
    // must have non-empty key
    throw ERR_BAD_ARGS;
}

After:

if (!p.canHaveKey && key.length > 0) {
    // skip
    continue;
}
if (p.mustHaveKey && key.length === 0) {
    // must have a key
    throw ERR_BAD_ARGS;
}

yeah, this is a valid point, but i meant more trying to avoid name clashes... as the data-* first framework they can change if there is a "real" clash. this change makes sense to me

[bencroker]

We agreed that we want to keep the possibility of using data-class-* attributes, so closing this.

[bencroker]

Reopening to explore whether there’s a case for not throwing an error, for better compatibility with other libraries.

[bencroker]

Decided that we’re going with throwing errors instead of failing silently, so the only way forward for allowing data-show-* attributes for other use-cases is to create a custom Datastar bundle that does not include the show plugin.