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

Fix EZP-21963: create separate "promise-based" build #32

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

wiseman17
Copy link
Contributor

JIRA issue: https://jira.ez.no/browse/EZP-21963

Done.

  • Additionaly, I have created separate page for manual testing of the promise-based version, since it became a non-trival matter to include it into a project, and also as means of "integration testing" for the promise-based version and it's external dependency.
  • Updated readme a bit
  • I had to reconfigure the unit tests, since the Q dependency is not mentioned in define call now.
    I'm not 100% sure, that window.Q = require("../../node_modules/q/q"); is a nice fix, but it works and looks quite nice to me :) @jakobwesthoff, your opinion is welcome here. May be I've even chosen a wrong way to get rid of "manifested" Q dependency?

is intended for general use.

The `PromiseCAPI.js` is a promise-based version of the library
which has a dependency on the Q library. While using this version a developer himself is responsible for inclusion of Q library into the project.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to ease reviews (and reading of the raw markdown file), can you please configure your editor to limit the line length at less than 80 characters

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(this comment also meant that you need to cut those lines, I'm sure you editor has a magic shortcut for that)

@dpobel
Copy link
Contributor

dpobel commented Nov 19, 2013

In addition to the inline comments, the promise versions should be mentioned in the installation paragraph of the README

@wiseman17
Copy link
Contributor Author

Fixes and additions implemented.

@@ -0,0 +1,3 @@
{
"directory": "test/manual/jsRestClientBundle/Resources/public/js/"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would put this file in test/manual/jsRestClientBundle (and adapt the README) as this is needed by the bundle only.

@wiseman17
Copy link
Contributor Author

Fixed

@wiseman17
Copy link
Contributor Author

@dpobel, is it ok to merge this one?
Taking into account your comment about separate commit, of course.

@jakobwesthoff
Copy link
Contributor

I am not 100% sure, why you needed to take out the Q dependency and replace it with the window.Q hack. Can you elaborate on this?

@wiseman17
Copy link
Contributor Author

Jakob, I've made it when I was trying to get rid of included Q and achieve "flexibility" you were mentioning here: #25
Partial quote:

The reason for not including Q (even int the promise build) was to give the user of the library the highest possible flexibility regarding choice of the used package management (requirejs, manual download, commonjs, ...).

I can't see a way for developer to include Q into project as a manual download without using global Q variable as it is done now. And hence - "window.Q" approach.
But, as I said, quite possible I've chosen the wrong way, and your advises on more proper approach are very welcome!

@jakobwesthoff
Copy link
Contributor

Ahh I see the reason now. I might have created a wrapper for the dependency, which is AMD compatible, to only use require.js compatible dependency management inside of the CAPI code itself:

define("q", [], function() {
    return window.Q;
});

I think this is a little bit cleaner, than using window.Q directly. Otherwise the changes seem okay to me :)

"q wrapper" approach to removing q dependency.
@wiseman17
Copy link
Contributor Author

@jakobwesthoff, did I get you right? See my latest commit, please.

@sonarcloud
Copy link

sonarcloud bot commented Jul 1, 2021

SonarCloud Quality Gate failed.

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot E 1 Security Hotspot
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants