Prevent JsCallback chain to be evaluated twice (#2150)

atk4 · Jan 30, 2024 · 9f5104c · 9f5104c
1 parent 595e3cd
commit 9f5104c
Show file tree

Hide file tree

Showing 15 changed files with 34 additions and 38 deletions.
diff --git a/demos/data-action/factory-view.php b/demos/data-action/factory-view.php
@@ -58,7 +58,7 @@ protected function getCardButton(Model\UserAction $action)
 $country = $country->loadBy($country->fieldName()->iso, 'fr');
 $country->name .= ' NO RELOAD';
 // suppress dirty field exception
-// https://github.com/atk4/data/blob/35dd7b7d95909cfe574b15e32b7cc57c39a16a58/src/Model/UserAction.php#L164
+// https://github.com/atk4/data/blob/35dd7b7d95/src/Model/UserAction.php#L164
 unset($country->getDirtyRef()[$country->fieldName()->name]);
 
 $cardActions = Card::addTo($app, ['useLabel' => true, 'executorFactory' => new $myFactory()]);

diff --git a/docs/callbacks.md b/docs/callbacks.md
@@ -342,11 +342,10 @@ $label->on('click', function (Jquery $j, $arg1) {
 ## Referring to event origin
 
 You might have noticed that JsCallback now passes first argument ($j) which so far, we have ignored. This argument is a
-jQuery chain for the element which received the event. We can change the response to do something with this element.
-Instead of `return` use:
+jQuery chain for the element which received the event. We can change the response to do something with this element like:
 
 ```
-$j->text('width is ' . $arg1);
+return $j->text('width is ' . $arg1);
 ```
 
 Now instead of showing an alert box, label content will be changed to display window width.

diff --git a/js/src/setup-fomantic-ui.js b/js/src/setup-fomantic-ui.js
@@ -32,7 +32,7 @@ const fomanticServicesMap = {
 // https://github.com/fomantic/Fomantic-UI/issues/2526
 $.extend = $.fn.extend = new Proxy($.fn.extend, { // eslint-disable-line no-multi-assign
     apply: function (target, thisArg, args) {
-        // https://github.com/fomantic/Fomantic-UI/blob/c30ed51ca12fc1762b04c2fd1a83d087c0124d07/src/definitions/behaviors/api.js#L48
+        // https://github.com/fomantic/Fomantic-UI/blob/c30ed51ca1/src/definitions/behaviors/api.js#L48
         const firstIndex = args[0] === true ? 1 : 0;
         const secondIndex = args[0] === true ? 2 : 1;
         if (args.length >= (args[0] === true ? 3 : 2)

diff --git a/public/js/atkjs-ui.js b/public/js/atkjs-ui.js
@@ -3590,7 +3590,7 @@ const fomanticServicesMap = {
 (external_jquery__WEBPACK_IMPORTED_MODULE_0___default().extend) = (external_jquery__WEBPACK_IMPORTED_MODULE_0___default().fn).extend = new Proxy((external_jquery__WEBPACK_IMPORTED_MODULE_0___default().fn).extend, {
   // eslint-disable-line no-multi-assign
   apply: function (target, thisArg, args) {
-    // https://github.com/fomantic/Fomantic-UI/blob/c30ed51ca12fc1762b04c2fd1a83d087c0124d07/src/definitions/behaviors/api.js#L48
+    // https://github.com/fomantic/Fomantic-UI/blob/c30ed51ca1/src/definitions/behaviors/api.js#L48
     const firstIndex = args[0] === true ? 1 : 0;
     const secondIndex = args[0] === true ? 2 : 1;
     if (args.length >= (args[0] === true ? 3 : 2) && external_jquery__WEBPACK_IMPORTED_MODULE_0___default().isPlainObject(args[firstIndex]) && external_jquery__WEBPACK_IMPORTED_MODULE_0___default().isEmptyObject(args[firstIndex]) && external_jquery__WEBPACK_IMPORTED_MODULE_0___default().isPlainObject(args[secondIndex])) {

diff --git a/public/js/atkjs-ui.js.map b/public/js/atkjs-ui.js.map
diff --git a/public/js/atkjs-ui.min.js.map b/public/js/atkjs-ui.min.js.map
diff --git a/src/App.php b/src/App.php
@@ -506,7 +506,7 @@ public function setResponseHeader(string $name, string $value): self
 
     /**
      * Will perform a preemptive output and terminate. Do not use this
-     * directly, instead call it form Callback, JsCallback or similar
+     * directly, instead call it from Callback, JsCallback or similar
      * other classes.
      *
      * @param string|StreamInterface|array $output Array type is supported only for JSON response

diff --git a/src/Card.php b/src/Card.php
@@ -234,8 +234,8 @@ public function addClickAction(Model\UserAction $action, Button $button = null,
         }
 
         if ($cardDeck !== null) {
-            // mimic https://github.com/atk4/ui/blob/3c592b8f10fe67c61f179c5c8723b07f8ab754b9/src/Crud.php#L140
-            // based on https://github.com/atk4/ui/blob/3c592b8f10fe67c61f179c5c8723b07f8ab754b9/src/UserAction/SharedExecutorsContainer.php#L24
+            // mimic https://github.com/atk4/ui/blob/3c592b8f10/src/Crud.php#L140
+            // based on https://github.com/atk4/ui/blob/3c592b8f10/src/UserAction/SharedExecutorsContainer.php#L24
             $isNew = !isset($cardDeck->sharedExecutorsContainer->sharedExecutors[$action->shortName]);
             if ($isNew) {
                 $ex = $cardDeck->sharedExecutorsContainer->getExecutorFactory()->createExecutor($action, $cardDeck->sharedExecutorsContainer);

diff --git a/src/Form.php b/src/Form.php
@@ -534,7 +534,7 @@ public function setupAjaxSubmit(): void
 
         // fix remove prompt for dropdown
         // https://github.com/fomantic/Fomantic-UI/issues/2797
-        // [name] in selector is to suppress https://github.com/fomantic/Fomantic-UI/commit/facbca003cf0da465af7d44af41462e736d3eb8b console errors from Multiline/vue fields
+        // [name] in selector is to suppress https://github.com/fomantic/Fomantic-UI/commit/facbca003c console errors from Multiline/vue fields
         $this->on('change', '.field input[name], .field textarea[name], .field select[name]', $this->js()->form('remove prompt', new JsExpression('$(this).attr(\'name\')')));
 
         if (!$this->canLeave) {

diff --git a/src/JsCallback.php b/src/JsCallback.php
@@ -6,7 +6,6 @@
 
 use Atk4\Ui\Js\Jquery;
 use Atk4\Ui\Js\JsBlock;
-use Atk4\Ui\Js\JsChain;
 use Atk4\Ui\Js\JsExpression;
 use Atk4\Ui\Js\JsExpressionable;
 
@@ -87,14 +86,12 @@ public function set($fx = null, $args = null)
 
             $response = $fx($chain, ...$values);
 
-            if (count($chain->_chain) === 0) {
-                // TODO should we create/pass $chain to $fx at all?
-                $chain = null;
-            } elseif ($response) {
-                // TODO throw when non-empty chain is to be ignored?
+            // TODO should we create/pass $chain to $fx at all?
+            if (count($chain->_chain) !== 0 && !$response instanceof JsExpressionable) {
+                throw new Exception('Jquery JsCallback chain was mutated but not returned');
             }
 
-            $ajaxec = $response ? $this->getAjaxec($response, $chain) : null;
+            $ajaxec = $this->getAjaxec($response);
 
             $this->terminateAjax($ajaxec);
         });
@@ -106,17 +103,16 @@ public function set($fx = null, $args = null)
      * A proper way to finish execution of AJAX response. Generates JSON
      * which is returned to frontend.
      *
-     * @param string|null                        $ajaxec
      * @param ($success is true ? null : string) $msg     General message, typically won't be displayed
      * @param bool                               $success Was request successful or not
      */
-    public function terminateAjax($ajaxec, $msg = null, bool $success = true): void
+    public function terminateAjax(JsBlock $ajaxec, $msg = null, bool $success = true): void
     {
         $data = ['success' => $success];
         if (!$success) {
             $data['message'] = $msg;
         }
-        $data['atkjs'] = $ajaxec;
+        $data['atkjs'] = $ajaxec->jsRender();
 
         if ($this->canTerminate()) {
             $this->getApp()->terminateJson($data);
@@ -126,18 +122,16 @@ public function terminateAjax($ajaxec, $msg = null, bool $success = true): void
     /**
      * Provided with a $response from callbacks convert it into a JavaScript code.
      *
-     * @param JsExpressionable|View|string|null $response response from callbacks,
-     * @param JsChain                           $chain
+     * @param JsExpressionable|View|string|null $response
      */
-    public function getAjaxec($response, $chain = null): string
+    public function getAjaxec($response): JsBlock
     {
         $jsBlock = new JsBlock();
-        if ($chain !== null) {
-            $jsBlock->addStatement($chain);
+        if ($response) {
+            $jsBlock->addStatement($this->_getProperAction($response));
         }
-        $jsBlock->addStatement($this->_getProperAction($response));
 
-        return $jsBlock->jsRender();
+        return $jsBlock;
     }
 
     #[\Override]

diff --git a/src/JsSse.php b/src/JsSse.php
@@ -82,21 +82,23 @@ public function send(JsExpressionable $action, bool $success = true): void
     {
         if ($this->browserSupport) {
             $ajaxec = $this->getAjaxec($action);
-            $this->sendEvent('js', $this->getApp()->encodeJson(['success' => $success, 'atkjs' => $ajaxec]), 'atkSseAction');
+            $this->sendEvent('js', $this->getApp()->encodeJson(['success' => $success, 'atkjs' => $ajaxec->jsRender()]), 'atkSseAction');
         }
     }
 
     /**
      * @return never
      */
     #[\Override]
-    public function terminateAjax($ajaxec, $msg = null, bool $success = true): void
+    public function terminateAjax(JsBlock $ajaxec, $msg = null, bool $success = true): void
     {
+        $ajaxecStr = $ajaxec->jsRender();
+
         if ($this->browserSupport) {
-            if ($ajaxec) {
+            if ($ajaxecStr !== '') {
                 $this->sendEvent(
                     'js',
-                    $this->getApp()->encodeJson(['success' => $success, 'atkjs' => $ajaxec]),
+                    $this->getApp()->encodeJson(['success' => $success, 'atkjs' => $ajaxecStr]),
                     'atkSseAction'
                 );
             }
@@ -105,7 +107,7 @@ public function terminateAjax($ajaxec, $msg = null, bool $success = true): void
             $this->getApp()->terminate();
         }
 
-        $this->getApp()->terminateJson(['success' => $success, 'atkjs' => $ajaxec]);
+        $this->getApp()->terminateJson(['success' => $success, 'atkjs' => $ajaxecStr]);
     }
 
     /**

diff --git a/src/UserAction/JsCallbackExecutor.php b/src/UserAction/JsCallbackExecutor.php
@@ -87,7 +87,7 @@ public function jsExecute(array $urlArgs = []): JsBlock
     #[\Override]
     public function executeModelAction(): void
     {
-        $this->invokeFxWithUrlArgs(function () { // backup/restore $this->args mutated in https://github.com/atk4/ui/blob/8926412a31bc17d3ed1e751e67770557fe865935/src/JsCallback.php#L71
+        $this->invokeFxWithUrlArgs(function () { // backup/restore $this->args mutated in https://github.com/atk4/ui/blob/8926412a31/src/JsCallback.php#L71
             $this->set(function (Jquery $j, ...$values) {
                 $id = $this->getApp()->uiPersistence->typecastLoadField(
                     $this->action->getModel()->getField($this->action->getModel()->idField),

diff --git a/src/View.php b/src/View.php
@@ -911,6 +911,7 @@ public function jsReload($args = [], $afterSuccess = null, $apiConfig = []): JsE
      *   if (!$data['clickable']) {
      *      return new JsExpression('alert([])', ['This record is not clickable'])
      *   }
+     *
      *   return $js->parent()->hide();
      * });
      *

diff --git a/tests-behat/crud.feature b/tests-behat/crud.feature
@@ -19,7 +19,7 @@ Feature: Crud
     Then I should not see "No records"
 
   Scenario: add after search and sort
-    # cover https://github.com/atk4/ui/commit/d42b07fbcc340d4e24f87056ddafdb94036c3cfa
+    # cover https://github.com/atk4/ui/commit/d42b07fbcc
     # TODO generalize JS reload with component reload
     When I click using selector "//th.sortable[//div[text()='Name']]"
     Then I should see "United Kingdom"

diff --git a/tests/FormTest.php b/tests/FormTest.php
@@ -91,13 +91,13 @@ protected function assertFormSubmit(\Closure $createFormFx, array $postData, \Cl
 
         if ($checkExpectedErrorsFx !== null) {
             self::assertFalse($wasSubmitCalled, 'Expected submission to fail, but it was successful!');
-            self::assertNotSame('', $res['atkjs']); // will output useful error
+            self::assertNotEmpty($res['atkjs']);
             $this->formError = $res['atkjs'];
 
             $checkExpectedErrorsFx($res['atkjs']);
         } else {
             self::assertTrue($wasSubmitCalled, 'Expected submission to be successful but it failed');
-            self::assertNull($res['atkjs']);
+            self::assertSame('', $res['atkjs']);
         }
     }
-Original file line number
+Diff line change
@@ Expand Up @@
          *   if (!$data['clickable']) {
          *      return new JsExpression('alert([])', ['This record is not clickable'])
          *   }
+         *
          *   return $js->parent()->hide();
          * });
          *
@@ Expand Down @@