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

add ability to serve angularjs templates w/ accompanying data (for example, for datatable visualizations) #6419

Closed
diosmosis opened this issue Oct 11, 2014 · 17 comments
Labels
duplicate For issues that already existed in our issue tracker and were reported previously.
Milestone

Comments

@diosmosis
Copy link
Member

As title. This is one of the main things stopping us from getting rid of twig. As long as data needs to be served w/ templates, there has to be PHP code and more specifically, controller code that uses a twig view.

Here is one possible way to remove the need for PHP controllers:

Create a special include directive, like piwik-include:

<div piwik-include="plugins/MyPlugin/mytemplate.html"
        piwik-api-my-data-attr="'method=MyPlugin.getMyData'"/>

The directive would get all attributes prefixed w/ piwik-api and do a bulk fetch for the API data that includes a request to get the template, eg,

method: API.bulkFetch,
urls: [
    'plugins/MyPlugin/mytemplate.html'
    '?method=MyPlugin.getmyData',
]

When the request is finished, the attribute values are replaces w/ the JSON encoded data of API responses.

@mattab mattab added this to the Mid term milestone Oct 12, 2014
@tsteur
Copy link
Member

tsteur commented Oct 12, 2014

BTW: We would get rid of Twig by using Angular routes #6037 . Once this is done all needed data would be requested via XHR. Would personally still let the browser fetch the HTML templates directly and let the server serve the HTML templates. Would be better for caching and reduce possible failures in case an API fails the template would be still served etc.

Would be awesome to get routes into Piwik and start removing twig

@diosmosis
Copy link
Member Author

@tsteur Could you describe how routes would allow fetching data for directives? Or will directives have to call piwikApi manually? Btw, I was thinking about adding CliMulti support for API.bulkFetch which would decrease the amount of data loss should a single request fail and decrease the amount of AJAX requests created by the browser (which, if templates and all data will be retrieved via by AJAX, may go up a lot in the near future).

@tsteur
Copy link
Member

tsteur commented Oct 13, 2014

Data fetched would be done in Angular controller. In routes you can even define what data should be loaded/resolved before rendering the templates and directives, see for instance http://stackoverflow.com/a/11972028

@diosmosis
Copy link
Member Author

That looks interesting, though it seems for individual directives, API requests would have to be done by the directives themselves? I can see how this would remove the need for twig, though.

I would still support making bulkFetch use CliMulti and be easy to use in JS to cut down on the amount of AJAX requests made.

@mattab
Copy link
Member

mattab commented Oct 13, 2014

making bulkFetch use CliMulti and be easy to use in JS to cut down on the amount of AJAX requests made

original idea to use CliMulti for bulk but seems maybe wrong. Bulk is supported in API: http://developer.piwik.org/api-reference/reporting-api#advanced-users-send-multiple-api-requests-at-once so we could use this somehow.

IMHO number of Ajax requests is not a big problem as requests should be cached by browser with proper caching headers. if I understand correctly then we wouldn't need this and instead can use AngularJS routes #6037 - please reopen if i'm wrong 👍

@mattab mattab closed this as completed Oct 13, 2014
@mattab mattab added the duplicate For issues that already existed in our issue tracker and were reported previously. label Oct 13, 2014
@diosmosis
Copy link
Member Author

caching is a problem when getting requests through the API since, 1) all requests made by the piwik UI are POSTs 2) Piwik doesn't provide caching. caching for HTML templates is fine, I think.

I don't know what your first paragraph means. I was suggesting to use bulk requests (from API) and use CliMulti in Piwik so requests are handled concurrently instead of sequentially. Could be specified w/ async=1 query parameter.

@diosmosis
Copy link
Member Author

@mattab ping

@mattab
Copy link
Member

mattab commented Oct 14, 2014

  1. So far it is by design that our API does not do caching since responses are supposed to be fast. Maybe we need to add caching though at some point if we start hitting API more and more from the UI.

What's the purpose of this ticket, is it to discuss how to get rid of twig? I'm not sure I get it, feel free to re-open if it's important and the issue discussed here won't be solved with AngularJS routes. cheers

@tsteur
Copy link
Member

tsteur commented Oct 14, 2014

Bulk requests can be used IMO if a directive has multiple resources to fetch to reduce the number of requests. They should not be combined over multiple directives to keep directives maintainable and independent. Not sure how many requests are sent max by one directive, maybe not so many?

Sending requests in parallel is basically already done by the browser (depends on the browser how many) so I do not really see a need to use CliMulti here. Especially since CliMulti doesn't work on all servers anyway (like not when using fpm, when using windows, ...) and it only sends a certain amount of requests in parallel.

Ideally, at some point we have a REST API which sends proper Expired / Not modified headers and we actually can cache requests in the browser. In this case single requests would be better as they can be better cached by the browser.

@diosmosis
Copy link
Member Author

Supporting caching in the API fixes the issue I was thinking about. Although it would mean not using POSTs for every request.

@tsteur
Copy link
Member

tsteur commented Oct 14, 2014

Ideally with REST API we will use correct verbs and not always POST. Having a true REST API would be so nice for Angular usage as we would get some features just out of the box

@diosmosis
Copy link
Member Author

I know, but POST was added for some security fix in the past having to do w/ token auths in the browser cache, so that will have to be dealt w/.

@mattab
Copy link
Member

mattab commented Oct 16, 2014

that's a legit point: token_auth should be POSTed for advanced security (not leaking them in browser cache or on HTTP requests)... it will be interesting to see how it could work with REST and caching

@diosmosis
Copy link
Member Author

I think maybe sessions can be used, and for some requests, the token auth is probably not necessary. For those, we can just switch to GET (and maybe put a check in piwikApi where an exception is thrown if the token_auth query param is used w/ the HTTP GET verb).

@tsteur
Copy link
Member

tsteur commented Oct 16, 2014

we will be able to workaround that... see for instance http://restcookbook.com/Basics/loggingin/ etc.
Most API's are based on REST and respect the proper HTTP methods ;)

Actually beside Piwik I can't think of any API that I've used in the last years that was not RESTful

@diosmosis
Copy link
Member Author

Hoorah for oauth!

@mattab
Copy link
Member

mattab commented Oct 16, 2014

hmac with use-once nonce sounds good, didn't hear of this idea before! oAuth also or instead would make sense too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
duplicate For issues that already existed in our issue tracker and were reported previously.
Projects
None yet
Development

No branches or pull requests

3 participants