Order independent testing

[jmstevers]

Splits events by \n\n\n, sorts the lines inside each event, and then checks the diff between normalized files.

I'd like @Superpat to look at this before I mark it ready for review.

[Superpat]

Will try to look at this tomorrow ! Thank you for fixing this.

[Superpat]

@jmstevers I did read it over quickly and am right in believing that this removes all ordering of the datalines ? For instance fragment lines should always be at the end. And sdk's should keep the ordering of the fragment lines they are sent.

[jmstevers]

Nope, data lines can be anywhere in the event (sse spec). Data lines are just appended to a buffer whenever they are read. I am concerned about the html being messed up when they are sorted, but I thought that you would have to be going out of your way to slice up the fragment string and reorder it.

Yep, you are right. I forgot that there are data lines other than fragments. I'll have to break up the data lines to ensure fragments are always at the end and that they are in the correct order.

[Superpat]

Hmm I dont know anymore, I'm looking at the sse parsing code and the event is parsed before the specific event handlers for mergeSignals and etc even run, so theoretically you where right. I just want to ask @delaneyj just to be sure.

In any case, we would still need to make sure the order of fragments is not lost.

[jmstevers]

Okay after reading the source code, each key in the data line (like fragments) is added to a map if it's not already in the map. If it's already in the map then its value is appended to the value in the map. So if im correct then this is valid.

data: fragments <div>
event: datastar-merge-fragments
data: fragments hello
data: mergeMode append
data: fragments </div>

But I'll do some further testing

[jmstevers]

Yep, this works. So for the tests I will just ensure fragments are in the correct order by appending them to a buffer whenever they appear and leaving them unsorted in the normalized text file.

[jmstevers]

@delaneyj Can you confirm if this is intended behavior?

[bencroker]

@jmstevers please clarify the question so I can answer.

[jmstevers]

Is it valid for sdks to send down this?

data: fragments <div>
event: datastar-merge-fragments
data: fragments hello
data: mergeMode append
data: fragments </div>

D* reads it correctly but it's kind of a messed up format. I worded my original question poorly...

[bencroker]

No. The order is defined. The SDK spec states that:

When called the function must write to the response buffer the following in specified order.

See https://github.com/starfederation/datastar/blob/develop/sdk/README.md#logic

[jmstevers]

So strict ordering is being enforced? I thought this was decided against in the discord, hence this PR.

IMO it's an unnecessary restriction and conflicts with how some frameworks order their events.

[bencroker]

Reading directly from the SDK spec, the order must follow this format. What about this is unclear?

event:
id:
retry:
data:

[jmstevers]

It's unclear why the spec requires it.

[Superpat]

@bencroker yeah the actual library code accepts any order for those lines. @delaneyj said he could make the code a bit simpler if sdk's did abide by the strict ordering of the test suite (that is even more strict then what the spec says), but I dont know it it's worth it.

[bencroker]

It's unclear why the spec requires it.

Purely for consistency’s sake. It’s similar to how we require that functions follow a naming convention. We want all SDK APIs and SSE event output to look alike. Note that the data lines can be in any order.

Unless you can provide a convincing argument to loosen the ordering, I don’t see any reason to change it.

@bencroker yeah the actual library code accepts any order for those lines. @delaneyj said he could make the code a bit simpler if sdk's did abide by the strict ordering of the test suite (that is even more strict then what the spec says), but I dont know it it's worth it.

The client intentionally works with looser ordering because using an SDK is optional.

[jmstevers]

1. Consistency with SSE spec
2. Less work/mental load for sdk developers (however small)
3. Simpler spec (however small)
4. Python and Ruby (in some cases) currently fail the tests due to ordering of event, id, retry. This would "fix" those instantly (not that it's that hard to fix)

Why is SSE output looking alike wanted? If the library enforced strict ordering and the code was simpler because of it, I would understand. But it would only be a few lines shorter from what I've seen.

Also the data lines can not be in any order. According to the spec, fragment and script data lines must be written last (I am also questioning why this is).

Is it also required to have a space after the colon for fields?

I will modify the script to only sort non fragment/script/header fields for now so we can get this closed.

[bencroker]

Why is SSE output looking alike wanted?

So that regardless of whether I use the Go SDK or the PHP SDK, the SSE events produced will look alike. This is just standardisation for the sake of consistency when comparing SDK output, troubleshooting issues, etc.

Maybe @delaneyj can weigh in on anything I’m missing.

Also the data lines can not be in any order. According to the spec, fragment and script data lines must be written last (I am also questioning why this is).

That’s true, the spec does indeed say that options should precede the fragments data line.

Is it also required to have a space after the colon for fields?

According to the SDK spec, it is.

[delaneyj]

SSE spec doesn't care about the order. Technically I don't either. I do think however there is value in being consistent. So every language and SDK looks the same and we aren't having to do crazy decodes each time. That's it, it's not a technical issue but a learning/documentation one IMO. And with that, I leave to y'all cause the client doesn't care.

[Superpat]

So to recap:

event
id
retry

data: ...options (this is the only part where ordering does not matter)

data: data (which can be path, selector, script or fragment, depending on the event).

[bencroker]

Correct!

[jmstevers]

2. The function must include the selector in the event data in the format selector SELECTOR\n.

Selector is not required to be at the end of the data lines.

[bencroker]

If there is a mistake, PR please.

[jmstevers]

Is this a mistake? RemoveFragments does not require data: selector to be last according to the spec.

[Superpat]

I just assumed it was treated like all the other mandatory datalines

[bencroker]

Looks good to me! Can you please review and approve, @Superpat?

[Superpat]

My awk is not that great, but I think it looks good. I'll run it in the console to try it out.

[bencroker]

Where are we with this?

[Superpat]

I will finish this review before the weekend is over. Still gotta finish setting up my work env after my ssd meltdown from last week..

[Superpat]

@bencroker this works great. @jmstevers can you simply update the readme? since the warning about strict ordering is no longer necessary.

[Superpat]

@jmstevers can you also add sdk/test/**/**/norm*.txt to the git ignore ?

[bencroker]

Merged, thank you both!