Migrate all jQuery promises to native ES6 `Promise` (ref: `$.Deferred()`, `$.when()`, `$.get()` and `$.post()`) #308

volterra79 · 2023-01-18T09:36:20Z

Depends on: #452

List of changes

replace $.Deferred::done() with Promise::then()
replace $.Deferred::fail() with Promise::catch()
replace $.Deferred::alwas() withPromise::finally()
replace $.when() with Promise::all()
replace $.get() with an internal utility function (ref: src/core/utils/utils/XHR::get())
replace $.post() with an internal utility function (ref: src/core/utils/utils/XHR::post())
~~add "util-deprecate" dependency to effectively report deprecation of some features to final developer (ref: src/deprecate.js)~~ add new file src/deprecate.js to effectively report deprecation of some features to final developer
add deprecation notice for g3wsdk.core.utils.resolve(value) function
add deprecation notice for g3wsdk.core.utils.reject(value) function

Closes: #82
Somehow related to: #98

…rejetcs()` methods

…one()` methods

Raruto · 2023-01-24T13:47:06Z

src/app/core/layers/map/vectorlayer.js

+        this.addFeatures(features);
+        resolve(features);
+      })
+      .catch(err => reject(err));


double check, isn't this a superfluous statement?

.catch(err => reject(err));

.catch(err => reject(err));

I didn't mark them all, but I found this kind of repetition quite often in the code

Raruto · 2023-01-24T14:09:42Z

src/app/core/layers/providers/kmlprovider.js

@@ -11,9 +11,7 @@ inherit(KMLDataProvider, DataProvider);
 const proto = KMLDataProvider.prototype;

 proto.getData = function() {
-  const d = $.Deferred();
-  return d.promise();
+  return new Promise((resolve, reject) => {})


double check, how can such a promise object be fulfilled?

return new Promise((resolve, reject) => {})

@Raruto i think that KMLDataProvider is never used. We need to double check and remove it

ok, i didn't check in too much detail, but still from some other substitutions you've done i guess your initial intent was to write something like this instead:

return Promise.resolve()

Raruto · 2023-01-24T14:23:01Z

src/app/core/layers/providers/qgisprovider.js

@@ -455,7 +457,7 @@ proto._loadVectorData = function(vectorLayer, bbox) {
      vectorLayer.setData(vectorDataResponse.vector.data);
      if (this._) return vectorDataResponse;
    })
-    .fail(() => {
+    .catch(() => {
      return false;
    })


double check, the following statement can be quite ambiguous:

.catch(() => { return false; })

it is best to make explicit what the intent is (ie. reject or resolve the promise) by using one of the following statements:

.catch(() => resolve(false))

.catch(() => reject(false))

@Raruto you can replace .catch(() => { return false; }) with .catch(() => reject(false)). We double check if this method is still used.

Raruto · 2023-01-24T14:27:49Z

src/app/core/layers/providers/wfsprovider.js

+      url,
+      data:params
+    }).then(response => resolve(response))
+      .catch(err => reject(err));


double check, written that way wouldn't the whole surrounding promise above be superfluous?

return // new Promise((resolve, reject) => { XHR.post({ ... }) // .then(response => resolve(response)) // .catch(err => reject(err)); // });

Raruto · 2023-01-24T14:33:02Z

src/app/core/layers/providers/wmsprovider.js

-    .finally(()=> clearTimeout(timeoutKey));
-  return d.promise();
+      })
+    } else this[METHOD]({url, layers, params})


😩 Damn weird!

else statement + missing brackets + a multiline istruction

Raruto · 2023-01-24T14:44:30Z

src/app/core/workflow/flow.js

-    // return a promise that will be reolved if all step go right
-    return d.promise();
+    return new Promise((resolve, reject) => {
+      //** Assign d module variable to an object having resolve, reject methods used on other module method


Specific code comments (jsdoc, but not only those) on the usefulness of a variable must be written when declaring it otherwise your development IDE is not able to recognize them, that is:

/** * Module variable having resolve, reject methods used on other module method */ let d; ... d = { resolve, reject };

Raruto · 2023-01-24T14:54:19Z

src/app/main.js

@@ -1164,7 +1164,7 @@ ApplicationTemplate.Services = {
  floatbar: null
 };

-ApplicationTemplate.fail = function({language='en', error }) {
+ApplicationTemplate.catch = function({language='en', error }) {


TODO: also double check that it hasn't been renamed elsewhere

Suggested change

ApplicationTemplate.catch = function({language='en', error }) {

ApplicationTemplate.fail = function({language='en', error }) {

Raruto · 2023-01-24T14:54:56Z

src/app/main.js

@@ -1205,7 +1205,7 @@ ApplicationService.init()
      if (error.responseJSON && error.responseJSON.error.data) error = error.responseJSON.error.data;
      else if (error.statusText) error = error.statusText;
    }
-    ApplicationTemplate.fail({
+    ApplicationTemplate.catch({


Suggested change

ApplicationTemplate.catch({

ApplicationTemplate.fail({

Raruto · 2023-01-24T15:01:57Z

src/services/data-query.js

@@ -135,7 +135,7 @@ function QueryService(){
      request.then(response => {
        const results = this.handleResponse(response, query);
        resolve(results);
-      }).fail(reject)
+      }).catch(reject)


double check, written like that the external promise seems superfluous:

return // new Promise((resolve, reject) => { request.then(response => { const results = this.handleResponse(response, query); resolve(results); }) // .catch(reject)

Raruto · 2023-01-24T15:02:51Z

src/services/data-search.js

@@ -44,7 +44,7 @@ function SearchService(){
          resolve({
            data
          })
-        }).fail(reject)
+        }).catch(reject)


same as: https://github.com/g3w-suite/g3w-client/pull/308/files#r1085451199

Raruto · 2023-02-03T09:41:57Z

src/app/core/layers/map/vectorlayer.js

+  return new Promise((resolve, reject) =>{
+    this.provider.getFeatures(options)
+      .then(features => {
+        this.addFeatures(features);
+        resolve(features);
+      })
+      .catch(err => reject(err));


Not sure what the previous behavior was, but now I think you need to add the return (resolve, reject) => this.provider.getFeatures from inside the promise to the outside to make . catch(err => reject(err)) also "catchable" from outside of this function (otherwise it will only throw an unhandled fatal error):

Suggested change

return new Promise((resolve, reject) =>{

this.provider.getFeatures(options)

.then(features => {

this.addFeatures(features);

resolve(features);

})

.catch(err => reject(err));

return new Promise((resolve, reject) =>

this.provider

.getFeatures(options)

.then(features => resolve(this.addFeatures(features)))

.catch(err => reject(err))

);

Anyway i'm not so sure about this statement, check it carefully too

Replace $.Deferred with new Promise

f2f7104

volterra79 added help wanted Extra attention is needed dependencies Pull requests that update a dependency file labels Jan 18, 2023

volterra79 added this to the v3.9 milestone Jan 18, 2023

volterra79 self-assigned this Jan 18, 2023

volterra79 requested a review from Raruto January 18, 2023 09:36

volterra79 added 24 commits January 18, 2023 10:43

Replace $.Deferred with new Promise

f67ed66

Replace $.Deferred with new Promise

65c9399

Replace $.Deferred with new Promise

ed7c43b

Replace $.Deferred with new Promise

7603140

Replace $.Deferred with new Promise

a8a62dd

Replace $.Deferred with new Promise

42f826b

Replace $.Deferred with new Promise

a1f9cb4

Replace $.Deferred with new Promise

12c6258

Replace $.Deferred with new Promise

4fded2e

Replace $.Deferred with new Promise

acd3404

Replace $.Deferred with new Promise

87c3a21

Replace $.Deferred with new Promise

4d5e298

Replace $.Deferred with new Promise

5c961cf

Replace $.Deferred with new Promise

bf06032

Replace $.Deferred with new Promise

de6424f

Replace $.Deferred with new Promise

370c791

Replace $.Deferred with new Promise

f98ceb1

Replace $.Deferred with new Promise

7cfb0a3

Replace $.Deferred with new Promise

76d1f69

Replace $.Deferred with new Promise

dfbe30a

Replace $.Deferred with Promise.resolve

17c6476

Replace $.Deferred with new Promise

adb68c6

Replace $.Deferred with new Promise

619b441

Replace $.Deferred with new Promise

68f2b94

Raruto changed the title ~~Migrate all jQuery $.Deferred promises to native ES6 Promise and $.get and $.post JQuery method with XHR object~~ Migrate all jQuery promises to native ES6 Promise (ref: $.Deferred, $.get and $.post) Jan 24, 2023

Raruto changed the title ~~Migrate all jQuery promises to native ES6 Promise (ref: $.Deferred, $.get and $.post)~~ Migrate all jQuery promises to native ES6 Promise (ref: $.Deferred(), $.get() and $.post() functions) Jan 24, 2023

Raruto changed the title ~~Migrate all jQuery promises to native ES6 Promise (ref: $.Deferred(), $.get() and $.post() functions)~~ Migrate all jQuery promises to native ES6 Promise (ref: $.Deferred(), $.when(), $.get() and $.post()) Jan 24, 2023

Raruto added 3 commits January 24, 2023 12:03

add deprecation warning for $.Deferred::resolve() and `$.Deferred::…

dc36bc2

…rejetcs()` methods

add deprecation notice for $.Deferred::promise() and `$.Deferred::d…

8aced3d

…one()` methods

reduce deprecation warning verbosity

2b0da39

Raruto reviewed Jan 24, 2023

View reviewed changes

Raruto added breaking Any code changes that are not or may not be backward compatible and removed dependencies Pull requests that update a dependency file refactoring Anything which could result in a API change labels Jan 31, 2023

Raruto mentioned this pull request Feb 2, 2023

Refactor app/gui/queryresults/queryresultsservice.js #317

Merged

Raruto reviewed Feb 3, 2023

View reviewed changes

add deprecation notice for $.get() and $.post()

92959b8

Raruto mentioned this pull request Mar 23, 2023

Remove promise return from function ApplicationService::changeProject({ gid, host }) #386

Draft

Merge branch 'dev' into replace-jquery-Deferred-with-Promise

419c113

Raruto modified the milestones: v3.9, v4.0 Jul 25, 2023

Raruto and others added 4 commits February 15, 2024 16:03

Merge branch 'dev' into replace-jquery-Deferred-with-Promise

c535906

replace almost all jquery promises

e8ca08e

wrong method

2269a40

Clean code

f7f4ba3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate all jQuery promises to native ES6 `Promise` (ref: `$.Deferred()`, `$.when()`, `$.get()` and `$.post()`) #308

Migrate all jQuery promises to native ES6 `Promise` (ref: `$.Deferred()`, `$.when()`, `$.get()` and `$.post()`) #308

volterra79 commented Jan 18, 2023 •

edited by Raruto

Raruto Jan 24, 2023

Raruto Jan 24, 2023

Raruto Jan 24, 2023

volterra79 Jan 24, 2023

Raruto Jan 24, 2023 •

edited

Raruto Jan 24, 2023

volterra79 Jan 24, 2023

Raruto Jan 24, 2023 •

edited

Raruto Jan 24, 2023

Raruto Jan 24, 2023 •

edited

Raruto Jan 24, 2023

Raruto Jan 24, 2023

Raruto Jan 24, 2023 •

edited

Raruto Jan 24, 2023

Raruto Feb 3, 2023 •

edited

	ApplicationTemplate.catch = function({language='en', error }) {
	ApplicationTemplate.fail = function({language='en', error }) {

Migrate all jQuery promises to native ES6 Promise (ref: $.Deferred(), $.when(), $.get() and $.post()) #308

Are you sure you want to change the base?

Migrate all jQuery promises to native ES6 Promise (ref: $.Deferred(), $.when(), $.get() and $.post()) #308

Conversation

volterra79 commented Jan 18, 2023 • edited by Raruto

Depends on: #452

List of changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Raruto Jan 24, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Raruto Jan 24, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Raruto Jan 24, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Raruto Jan 24, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Raruto Feb 3, 2023 • edited

Choose a reason for hiding this comment

Migrate all jQuery promises to native ES6 `Promise` (ref: `$.Deferred()`, `$.when()`, `$.get()` and `$.post()`) #308

Migrate all jQuery promises to native ES6 `Promise` (ref: `$.Deferred()`, `$.when()`, `$.get()` and `$.post()`) #308

volterra79 commented Jan 18, 2023 •

edited by Raruto

Raruto Jan 24, 2023 •

edited

Raruto Jan 24, 2023 •

edited

Raruto Jan 24, 2023 •

edited

Raruto Jan 24, 2023 •

edited

Raruto Feb 3, 2023 •

edited