Vojtech Szocs has posted comments on this change. Change subject: frontend: Introducing oVirtJS / GWT wrapper projects ......................................................................
Patch Set 9: (3 comments) https://gerrit.ovirt.org/#/c/33720/9/frontend/ovirtjs/ovirtjs-lib/src/main/services.js File frontend/ovirtjs/ovirtjs-lib/src/main/services.js: Line 4: export class Http { Line 5: Line 6: // Send HTTP request to remote server. Line 7: // Return `Promise` that resolves to HTTP response text. Line 8: // Implementation is based on `XMLHttpRequest` object. > Maybe in the future when we will have proper packaging for browser/node.js Agreed. Line 9: // - method: HTTP method to use, defaults to `GET`. Line 10: // - url: URL to send request to, defaults to empty string (document base URI). Line 11: // - headers: HTTP headers, defaults to empty object. Line 12: // - body: Any value supported by `XMLHttpRequest.send` function, defaults to null. Line 8: // Implementation is based on `XMLHttpRequest` object. Line 9: // - method: HTTP method to use, defaults to `GET`. Line 10: // - url: URL to send request to, defaults to empty string (document base URI). Line 11: // - headers: HTTP headers, defaults to empty object. Line 12: // - body: Any value supported by `XMLHttpRequest.send` function, defaults to null. > I believe Http.send() method should provide sufficient level of abstraction Current oVirtJS code calls Http.send method with "body" parameter being either string (created via JSON.stringify) or null. The "body" value normalization happens in transformHttpBody function, see resources.js. Accepting JSON object for "body" parameter is possible, but I'd rather avoid that, as it would mean extra responsibility for Http.send implementation (i.e. know how to stringify a JSON object). If JSON.stringify & JSON.parse aren't supported in other (non-browser) environments, we can add a dedicated service (e.g. ovirt.svc.Json) for this purpose. In general, I'd prefer having service-per-responsibility, instead of few services with too many responsibilities. As for the JSDoc, agreed. We'll move to JSDoc-style comments as soon as it starts supporting ES6 syntax: https://github.com/jsdoc3/jsdoc/issues/555 Good news (from issue tracker above) - JSDoc recently added support for classes and work on module support is in progress. So if everything goes well, once JSDoc 3.4.0 is out, we can start using it. (I'd rather avoid using JSDoc master right now because it's still in motion.) Line 13: static send ({ method, url, headers, body }) { Line 14: return new Promise((resolve, reject) => { Line 15: var xhr = new XMLHttpRequest(); Line 16: https://gerrit.ovirt.org/#/c/33720/9/frontend/ovirtjs/ovirtjs-lib/src/main/utils.js File frontend/ovirtjs/ovirtjs-lib/src/main/utils.js: Line 1: // Utility for working with objects. Line 2: export class ObjectUtil { > Not sure if we should go overboard with the desctructuring. It makes sense Well, the main reason for using it would be consistency across all methods and (arguably better) code readability since we'd have to specify parameters as object attributes, plus the ability to define structure (de-structure) of individual parameters. But yeah, in this case (internal utility module) it might not be necessary, I agree. It was just an idea. Line 3: Line 4: // Compute change between `original` and `modified` objects. Line 5: // Return diff object containing the difference. Line 6: // Diff object will be null if there is no change. -- To view, visit https://gerrit.ovirt.org/33720 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I2646646bc880446aa3035dd951e65c9af81933a1 Gerrit-PatchSet: 9 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Vojtech Szocs <[email protected]> Gerrit-Reviewer: Alexander Wels <[email protected]> Gerrit-Reviewer: Daniel Erez <[email protected]> Gerrit-Reviewer: Einav Cohen <[email protected]> Gerrit-Reviewer: Gilad Chaplik <[email protected]> Gerrit-Reviewer: Greg Sheremeta <[email protected]> Gerrit-Reviewer: Jenny Kang <[email protected]> Gerrit-Reviewer: Kanagaraj M <[email protected]> Gerrit-Reviewer: Lior Vernia <[email protected]> Gerrit-Reviewer: Martin Betak <[email protected]> Gerrit-Reviewer: Tomas Jelinek <[email protected]> Gerrit-Reviewer: Vojtech Szocs <[email protected]> Gerrit-Reviewer: Zuzana Svetlíková <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
