Skip to content
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

RFC simplifed JSON payload + named batches #142

Merged
merged 7 commits into from
Aug 21, 2021

Conversation

leastbad
Copy link
Contributor

@leastbad leastbad commented Jul 30, 2021

Change is transparent but incompatible with any stored v4 payloads.
Operations now fire in the order that they were created.
Mix and match multiple batches + non-batched operations in one broadcast.

[
  {
    "operation": "innerHtml",
    "html": "HTML 1",
    "selector": "#posts"
  },
  {
    "operation": "dispatchEvent",
    "html": "Event 1",
    "selector": "#posts",
    "batch": "my_custom_batch"
  },
  {
    "operation": "innerHtml",
    "html": "HTML 2",
    "selector": "#posts",
    "batch": "my_custom_batch"
  },
  {
    "operation": "dispatchEvent",
    "html": "Event 2",
    "selector": "#posts",
    "batch": "my_custom_batch"
  }
]

Pass batch: true or batch: "foo" as an option. cable-ready:batch-complete will be raised on document, with batch: 'foo' in the details object.

TODO: @marcoroth proposed a potential cable_ready.batch("foo") do..end syntax.
TODO: SR PR to follow See stimulusreflex/stimulus_reflex#536

@leastbad leastbad added enhancement proposal ruby Pull requests that update Ruby code javascript Pull requests that update Javascript code labels Jul 30, 2021
@leastbad leastbad added this to the 5.0 milestone Jul 30, 2021
@leastbad leastbad self-assigned this Jul 30, 2021
@marcoroth
Copy link
Member

marcoroth commented Jul 31, 2021

This is very exciting! A lot to love about this!

Here are some additional thoughts which just came to mind:

  1. I think we should introduce a version property in the payload, so we can make the migration smoother. Also potential apps can check against that payload version and print according a message about how to upgrade or even support the processing of both payload versions.
  2. What does the batch: true option? Shouldn't a batch always be named?
  3. What about a cable-ready:before-batch or cable-ready:batch-start event?
  4. I like the name cable-ready:batch-complete, but maybe cable-ready:after-batch would be more consistent?
  5. I think we can drop the cable_ready.batch("foo") do..end syntax, because the now proposed payload is much simpler than what we discussed before.

@leastbad
Copy link
Contributor Author

  1. StimulusReflex wraps its CableReady payload in an object with two keys: cableReady: true and operations: [] (which used to be operations: {}. We could add a version to the StimulusReflex broadcaster payload, but there's no outer object on the raw array to attach a version to. While I agree that a version would be a nice-to-have, it's like 5% convincing enough to abandon the currently proposed flat "array of objects" structure, which for me is one of the primary selling points of this migration.
  2. In the case of batch: true, true is the name! In other words, "yes".
  3. Could you suggest a use case for that? I have to admit that I'm highly skeptical that this would be useful, but it would require a fair bit more code complexity to support it. Right now the implementation to emit batch-complete is wonderfully simple and I'd like to keep it if possible.
  4. I prefer batch-complete which is different from the standard, per-option lifecycle events. I feel like it signifies something different, as opposed to after-batch which looks like just another operation. In fact, you literally could create an operation called batch and it would still work just fine with batch-complete!
  5. As you wish! We can also add it later.

Reading this, my feedback feels like a bunch of me saying "no". It's just a coincidence, I swear!

@marcoroth
Copy link
Member

  1. Right, I mixed that up with the StimulusReflex payload. But maybe it would still make sense to encode that into the CableReady payload as well, especially if we would change the payload again in the future. Not that I intend to, but you never know what the future brings.
  2. I see. I wonder if we should limit it to just strings are implicitly call #to_s on whatever is passed in.
  3. Hmm. I'm not too sure about a good use case for that, but I also can't name one for batches in general 🙈
  4. I'm with you on that and also thought about a custom operation named batched. Just wanted to bring it up.
  5. I won't miss it for now 😉

@leastbad
Copy link
Contributor Author

  1. How would you suggest approaching this? Do you want to mix a version into each object? Do you want the first element of the array to be a string? My skeptic bone is tingling, but I'm listening. (Note that as of right now, there's no contract suggesting that we'll maintain the JSON payload format. SR is the only library I'm aware of other than CR itself that parses the internal structure.)
  2. We don't attempt to validate any other option. What about batch has you particularly concerned? (FWIW: batch:true works fine! I tested it.)
  3. I appreciate your honesty! I like the idea of batches from the perspective of use cases people haven't thought of, yet. Meanwhile, @hopsoft seemed to have a more specific use case in mind! Perhaps he could share.
  4. 👍
  5. 👍

@marcoroth
Copy link
Member

  1. I thought about something like this for the CableReady payload:
{
  "payload_version": "5",
  "cable_ready": {
    "package_version": "5.0.0"
  },
  "operations": [
    {
      "operation": "innerHtml",
      "html": "HTML 1",
      "selector": "#posts"
    },
    // {...},
    // {...},
    // {...}
  ]
}

And similarly if it's then wrapped in StimulusReflex:

Variant 1 (re-function the cable_ready key and wrap the CableReady payload under it) Variant 2 (Keep the cable_ready key and wrap the CableReady payload under the operations key)
{
  "payload_version": "3",

  "stimulus_reflex": {
    "package_version": "3.5.0"
  },

  "cable_ready": {
    "payload_version": "5",
    "cable_ready": {
      "package_version": "5.0.0",
    },
    "operations": [
      {
        "operation": "innerHtml",
        "html": "HTML 1",
        "selector": "#posts"
      },
      // {...},
      // {...},
      // {...}
    ]
  },

  // ...
}
{
  "payload_version": "3",

  "stimulus_reflex": {
    "package_version": "3.5.0"
  },

  "cable_ready": true,

  "operations": {
    "payload_version": "5",
    "cable_ready": {
      "package_version": "5.0.0",
    },
    "operations": [
      {
        "operation": "innerHtml",
        "html": "HTML 1",
        "selector": "#posts"
      },
      // {...},
      // {...},
      // {...}
    ]
  },

  // ...
}

All the shown variants also include a package_version which could also be used for some sort of warnings. If you don't like them just ignore it :)

  1. 👍🏼
  2. 👍🏼

@leastbad
Copy link
Contributor Author

leastbad commented Aug 1, 2021

Marco, I really appreciate the energy that you've put into describing this... but this direction is a hard no for me.

I believe that the simplicity of what this PR achieves is a huge leap forward as it is currently structured.

The code required to parse and work with an array of objects is easy to read and understand, which is a major win.

StimulusReflex 3.5 depends on CableReady 5, so there is no concern about JSON schema versioning. We have never made any claim or suggestion that the structure of the JSON itself is never going to change. The change proposed in this PR is 100% transparent to the developer without any version being passed.

Supporting the previous JSON schema would mean SR would have to support the old version of CR, too. That's already not possible due to 3-4 merged SR features which rely on CR v5.

Sorry, I just can't get behind this one.


On the positive side of things... I might actually be able to simplify the SR implementation side even further.

In 3.4 we do this awkward thing where the StimulusReflex data structure might be in the detail or it might be part of the operation itself, depending on the morph type. It occurs to me that we should be able to consolidate one approach. It's a lot easier to work with this code now, since there's no need to compartmentalize operations into arbitrary silos based on operation type.

@leastbad leastbad merged commit 3c48f84 into stimulusreflex:master Aug 21, 2021
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 ruby Pull requests that update Ruby code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants