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 isFullRequest parameter to JS tracker queueRequest() … #19942

Merged
merged 4 commits into from Nov 15, 2022

Conversation

diosmosis
Copy link
Member

Description:

This PR adds the ability to use queueRequest() with a full tracker request URL (ie, the result of getRequest()). The primary use case is when there is asynchronous code that is used to determine the contents of the request, and we want to make sure the correct parameters are used when the async code finishes. For example:

function myPluginTrackSomething(..., customizeParameter) {
    var tracker = getAsyncTracker(); // only one tracker just for the example
    var url = tracker.getRequest(...);
    customizeParameter(..., function (newParam) {
        tracker.queueRequest(url + '&param=' + encodeURIComponent(newParam), true);
    });
}

Prevents the following type of error:

  1. user loads a SPA. myPluginTrackSomething() is called, which initiates the customizer callback
  2. user changes the page, track pageview is called
  3. customizer callback finishes and queueRequest is called. But since we use getRequest() beforehand, the URL uses the correct action name/URL, etc.

Review

…ant to call getRequest() ourselves before queueRequest()
@diosmosis diosmosis added the Needs Review PRs that need a code review label Nov 3, 2022
@diosmosis diosmosis added this to the 5.0.0 milestone Nov 3, 2022
@github-actions
Copy link
Contributor

This issue is in "needs review" but there has been no activity for 7 days. ping @matomo-org/core-reviewers

@github-actions github-actions bot added the Stale The label used by the Close Stale Issues action label Nov 11, 2022
@sgiehl
Copy link
Member

sgiehl commented Nov 11, 2022

@diosmosis would you solve the conflicts?

@diosmosis
Copy link
Member Author

@sgiehl updated

@diosmosis
Copy link
Member Author

added a test as well

@github-actions github-actions bot removed the Stale The label used by the Close Stale Issues action label Nov 12, 2022
Copy link
Member

@sgiehl sgiehl left a comment

Choose a reason for hiding this comment

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

The javascript tests are failing and might need to be updated. Otherwise this imho looks good.
Might be possible though that our documentation needs an update.

Copy link
Contributor

@peterhashair peterhashair left a comment

Choose a reason for hiding this comment

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

JS tests passed, not too sure that Redis tests are related, seems random.

@sgiehl sgiehl merged commit 9b006e1 into 5.x-dev Nov 15, 2022
@sgiehl sgiehl deleted the js-tracker-queue-request-whole-url branch November 15, 2022 06:50
@elabuwa elabuwa added the not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. label Nov 22, 2022
bx80 pushed a commit that referenced this pull request Nov 25, 2022
* add isFullRequest parameter to JS tracker queueRequest() in case we want to call getRequest() ourselves before queueRequest()

* add JS tracker tests

* update expected count of requests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review PRs that need a code review not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants