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

[Bug] Calling a custom js variable within a custom js variable within a custom js variable leads to an error #869

Closed
SW-Vincent opened this issue Aug 30, 2024 · 17 comments

Comments

@SW-Vincent
Copy link

Matomo Tag Manager recently added a feature to import variables within custom JS variables.

This feature is great overall : you can call a custom JS variable within a custom JS variable and while Matomo UI code will look like this :

function () { 
  var clickHostName = {{JS - Click hostname}};
  var targetHostName =  "dummy.example.com"
  if(targetHostName === targetHostName )){
    return true;
  }
  return false;
}

the code that is actually set by Matomo Tag Manager looks like this :

Templates['CustomJsFunctionVariable5630bd1c'] = (function () { return function (parameters, TagManager) { this.get = function () { 
  var clickHostName = TagManager._buildVariable({"name":"JS - Click hostname","type":"CustomJsFunction","lookUpTable":[],"defaultValue":"","parameters":{"jsFunction":"HERE-IS-A-STRING-FORMATTED-VERSION-OF-THE-FUNCTION"},"Variable":"CustomJsFunctionVariable4d0ada15"}, parameters.get('container')).get();
  var targetHostNames = "dummy.example.com" ;
  if(reservationsHostNames.includes(clickHostName)){
    return true;
  }
  return false;
}; } })(); 

Now we're getting to the bug : you noticed that the clickHostName variable is calling the customJSFunction as an object, with TagManager._buildVariable.parameters.jFunction being a string.
If within this string there is an other custom JS function call, the " characters within the function reference will escape the string, leading to an error "Unexpected identifier 'name'"
image

image

@snake14
Copy link
Contributor

snake14 commented Sep 1, 2024

Hi @SW-Vincent . Thank you for taking the time to create this issue. I'm not sure If I quite follow the issue. I tested nested Custom JS variables and they worked as expected. For example, I had variable 1 return a string value, variable 2 return the value of variable 1 concatenated with another string, and variable 3 return the value of variable 2 concatenated with a string. I didn't see any errors in debug mode or after publishing and variable 3 displayed the expected value.

@SW-Vincent
Copy link
Author

SW-Vincent commented Sep 2, 2024

Hi @snake14,

Considering the test you made, you understood the issue.

I reproduced your test with the following custom JS on 2 Matomo Cloud accounts :

function () { 
  return "abc"
}
function () { 
  //{{JS - const for debug - lvl 1}} function returns "abc"
  return {{JS - const for debug - lvl 1}} + "def"
}
function () { 
  //{{JS - const for debug - lvl 2}} concatenates {{JS - const for debug - lvl 1}} with "def"
  return {{JS - const for debug - lvl 2}} + "ghi"
}

The issue occured on one of the two Matomo Cloud accounts i ran the test on.
The difference between these accounts is that content security policy prevents the error somehow when partially bloquing the preview script (the console cannot acces the code od container_containerId_preview.js so the error is not displayed and the preview works as expected).

Note that for the issue occurs even when the variable is never called.

I took the risk of publishing the container on the account that didn't show the issue (where CSP blocks the preview) and here is what i get :

Templates['CustomJsFunctionVariable452dc43a'] = (function() {
            return function(parameters, TagManager) {
                this.get = function() {
                    //TagManager._buildVariable({"name":"JS - const for debug - lvl 2","type":"CustomJsFunction","lookUpTable":[],"defaultValue":"","parameters":{"jsFunction":"function () { \n  \/\/{{JS - const for debug - lvl 1}} function returns \"abc\"\n  return {{JS - const for debug - lvl 1}} + \"def\"\n}"},"Variable":"CustomJsFunctionVariabled69366c8"}, parameters.get('container')).get() concatenates TagManager._buildVariable({"name":"JS - const for debug - lvl 1","type":"CustomJsFunction","lookUpTable":[],"defaultValue":"","parameters":{"jsFunction":"function () { \n  return \"abc\"\n}"},"Variable":"CustomJsFunctionVariable73c91c00"}, parameters.get('container')).get() with "def"
                    return TagManager._buildVariable({
                        "name": "JS - const for debug - lvl 2",
                        "type": "CustomJsFunction",
                        "lookUpTable": [],
                        "defaultValue": "",
                        "parameters": {
                            "jsFunction": "function () { \n  \/\/{{JS - const for debug - lvl 1}} function returns \"abc\"\n  return {{JS - const for debug - lvl 1}} + \"def\"\n}"
                        },
                        "Variable": "CustomJsFunctionVariabled69366c8"
                    }, parameters.get('container')).get() + "ghi"
                }
                ;
            }
        }

There is no issue once published (as you can see the 1st level variable isn't decompliled within the 3rd level variable), so it may be a preview issue only.
This is still an issue though, as when it occurs preview doesn't work.

@snake14
Copy link
Contributor

snake14 commented Sep 2, 2024

@SW-Vincent That looks very similar to the test that I ran. However, I didn't see any errors in the JS console while in preview mode and the string output as expected. Maybe it's browser specific. I'm testing using the Brave browser. What browser are you testing with? Also, what's the rest of your environment (version of Matomo, ...) look like?

@SW-Vincent
Copy link
Author

SW-Vincent commented Sep 3, 2024

What browser are you testing with?

@snake14 I ran my previous tests on Chrome, with Matomo Helper Chrome extension active.
I just did the same tests on Microsoft Edge and the results are the same.

What's the rest of your environment (version of Matomo, ...) look like

I used Matomo Cloud accounts for all the tests so i don't keep track of the version.

@snake14
Copy link
Contributor

snake14 commented Sep 3, 2024

@SW-Vincent I'm still unable to reproduce the issue. I tested on a Matomo Cloud instance (altamash.matomo.cloud), created variables nearly identical to what you described above, and I can see the variables outputting the expected values in preview mode. I tried Brave, Firefox, Chrome, and Opera; All of the browsers that I tried worked as expected.

Any suggestions @AltamashShaikh ?

@AltamashShaikh
Copy link
Contributor

@SW-Vincent I tried creating 3 variables

  1. normal-nested
    function () { return "abc"; }
  2. normal-nested-lvl1
    function () { return {{normal-nested}} + "def"; }
  3. normal-nested-lvl2
    function () { return {{normal-nested-lvl1}} + "ghi"; }

I can see the following output in debug window

Screenshot from 2024-09-04 07-42-32

I published this changes and it seems to work fine for me too

Screenshot from 2024-09-04 07-43-13

Screenshot from 2024-09-04 07-45-20

@SW-Vincent
Copy link
Author

A few specifications about my case :

  • seems to happen only on preview ;
  • happens only when there is no CSP error ;

Can you confirm to me that there is no CSP error in your console ?
When there is a CSP error, preview still works as expected but the nested variables error doesn't occur (as its execution is bloqued by CSP) ;

@AltamashShaikh
Copy link
Contributor

@SW-Vincent No CSP error and I tested in both preview and publish mode.

@SW-Vincent
Copy link
Author

Then may i email you the matomo URL so we can check wether this is a Matomo Tag Manager issue or not ?

@AltamashShaikh
Copy link
Contributor

Then may i email you the matomo URL so we can check wether this is a Matomo Tag Manager issue or not ?

@SW-Vincent sure 👍

@AltamashShaikh
Copy link
Contributor

@SW-Vincent Can you check again ?
You would have to create a new Variable and test it, since the fix was deployed but we cannot fix the existing ones

@SW-Vincent
Copy link
Author

Hi @AltamashShaikh ,

I performed a new test and everything semms to work in both preview and live versions !

I noticed doing so that there is no error handling preventing you from deleting a variable (any type) that would be called within a custom JS variable (on the other hand, this kind of error handlin does exist for variables called in tags and triggers).

@AltamashShaikh
Copy link
Contributor

@SW-Vincent I didn't get you, any screenshots ?

@AltamashShaikh
Copy link
Contributor

@SW-Vincent Got your problem, thanks to @snake14, can you create a new ticket with the issue ? So that we can prioritise it

@SW-Vincent
Copy link
Author

@SW-Vincent Got your problem, thanks to @snake14, can you create a new ticket with the issue ? So that we can prioritise it

Just created #895

@AltamashShaikh
Copy link
Contributor

Thanks 👍 and can we close this issue now ? @SW-Vincent

@SW-Vincent
Copy link
Author

SW-Vincent commented Sep 25, 2024

Yes we can !

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

No branches or pull requests

3 participants