Batch action tweaks #5
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
weaverryan
commented
Sep 22, 2022
| $controller = $event->getController(); | ||
|
|
||
| if ($controller instanceof BatchActionController) { | ||
| if ($request->attributes->get('_is_live_batch_action')) { |
Author
There was a problem hiding this comment.
idk, just a preference - but super minor
| * B) We DO have a _mounted_component, so no need to hydrate, | ||
| * but we DO need to make sure it's set as the controller. | ||
| */ | ||
| if (!$request->attributes->has('_mounted_component')) { |
Author
There was a problem hiding this comment.
Once I dug in, I better understood the limitations and complexities that you were dealing with. This is really the only significant change. I wanted the "role" of onKernelController to be more straightforward - vs being the "traffic cop" that has whatever code is necessary to get the job done.
The job of onKernelController is now to do the following, and it only "acts" when the controller will be a method on the component (i.e. it's skipped for _batch):
A) Either mount the component (which is already set as the controller object. Or, if already mounted, make sure the mounted component IS the controller object.
B) Figure out the arguments to pass to the controller.
That feels more straightforward to me.
| 'actions' => [], | ||
| ]; | ||
| } | ||
| if (!$request->attributes->has('_live_request_data')) { |
Author
There was a problem hiding this comment.
Added this so that we could call this method ad-hoc as many times as we want (which helps make the above methods more clear) and not worry about performance.
weaverryan
force-pushed
the
batch-action-tweaks
branch
from
594ef00 to
0f37365
Compare
kbond
pushed a commit
that referenced
this pull request
Oct 14, 2022
kbond
pushed a commit
that referenced
this pull request
Oct 14, 2022
…th "queued" changes (weaverryan, kbond) This PR was merged into the 2.x branch. Discussion ---------- [WIP] [Live] Make Ajax calls happen in serial, with "queued" changes | Q | A | ------------- | --- | Bug fix? | yes | New feature? | yes | Tickets | None | License | MIT Hi! Consider the following situation: A) Model update is made: Ajax call starts B) An action is triggered BEFORE the Ajax call from (A) finishes. Previously, we would start a 2nd Ajax call for (B) before the Ajax call from (A) finished... meaning two Ajax calls were happening at the same time. There are a few problems with this: (i) Ajax call (B) is missing any potential data changes from Ajax call (A) and (i) complexity of multiple things loading at once, and potentially finishing in a different order. This PR simplifies things, which matches Livewire's behavior anyways. Now, the action from (B) will WAIT until the Ajax call from (A) finishes and THEN start. In fact, while an Ajax call is being made, all changes (potentially multiple model updates or actions) will be "queued" and then all set at once on the next request. TODO: * [X] Update the backend: for POST requests, the "data" was moved under a `data` key in JSON. Previously the entire body was the "data". * [X] Update the backend: for POST/action requests, action "arguments" were moved from query parameters to an `args` key in the JSON. * [x] Frontend: add tests for the `batch` action Ajax calls * [x] Update the backend: a new fake `/batch` action needs to be added that can handle multiple actions at once * [ ] A new `updatedModels` is sent on the ajax requests. If the signature fails, use this to give a better error message about what readonly properties were just modified. (in fact, this is the only purpose of sending this new field to the backend at this time). * [X] ~~Pass a `includeUpdatedModels` value to the Stimulus controller ONLY when in `dev` mode.~~ For consistency, we will always pass the `updatedModels` in our Ajax requests, though this is intended to be "internal" and we won't use it other than to throw better errors. * [X] ~~(Optional) If the backend has an unexpected exception (i.e. 5xx or maybe some 4xx where we mark that this is a problem we should show the developer), then render the entire HTML exception page (in dev mode only) so the user can see the error.~~ Done in symfony#467 Cheers! Commits ------- 4fbcebe cs fix and small tweaks a0c4d3f use `Request::toArray()` 13d7110 mildly reworking to follow the logic of what is happening more clearly (#5) b47ece1 [Live] add batch action controller 8b1e879 Refactoring towards single request & pending updates
kbond
pushed a commit
that referenced
this pull request
Nov 6, 2022
# This is the 1st commit message: WIP heavy refactoring to Component Initial "hook" system used to reset model field after re-render Adding a 2nd hook to handle window unloaded reinit polling after re-render Adding Component proxy # This is the commit message #2: fixing some tests # This is the commit message #3: Refactoring loading to a hook # This is the commit message #4: fixing tests # This is the commit message #5: rearranging # This is the commit message #6: Refactoring polling to a separate class
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Ignore your commit :p