Add recommended lite page js approach

[andrewscfc]

Resolves JIRA: NA

Summary

Adds recommended lite page js approach to Coding Standards to ensure inline JS we add is maintainable

Developer Checklist

• UX
  • UX Criteria met (visual UX & screenreader UX)
• Accessibility
  • Accessibility Acceptance Criteria met
  • Accessibility swarm completed
  • Component Health updated
  • P1 accessibility bugs resolved
  • P2/P3 accessibility bugs planned (if not resolved)
• Security
  • Security issues addressed
  • Threat Model updated
• Documentation
  • Docs updated (runbook, READMEs)
• Testing
  • Feature tested on relevant environments
• Comms
  • Relevant parties notified of changes

Testing

• Manual Testing required?
  • Local (Ready-For-Test, Local)
  • Test (Ready-For-Test, Test)
  • Preview (Ready-For-Test, Preview)
  • Live (Ready-For-Test, Live)
• Manual Testing complete?
  • Local
  • Test
  • Preview
  • Live

Additional Testing Steps

1. List the steps required to test this PR.

Useful Links

• Coding Standards
• Repository use guidelines
• Add Links to useful resources related to this PR if applicable.

[karinathomasbbc]

As above, this code has now moved on: imports
implementation - should we include these links instead?

Or should we include the links to the tests for the scripts (e.g.

• https://github.com/bbc/simorgh/blob/latest/src/server/utilities/liteATITracking/index.test.ts
• https://github.com/bbc/simorgh/blob/latest/src/server/utilities/liteATITracking/viewTracking/index.test.ts
• https://github.com/bbc/simorgh/blob/latest/src/server/utilities/liteATITracking/clickTracking/index.test.ts

[andrewscfc]

If folks agree with my approach re: the PR above, I'd leave it as is, let's discuss on this PR 👍

[HarveyPeachey]

Might be worth calling out, if the inline JS that we create imports another file, I don't think the transpilation will work properly, so that needs to be bared in mind. There might be a way we can configure babel and webpack to include imported libraries directly inline maybe? But atm it seems like it can't, which was an issue found doing the opera mini script beacon

simorgh/src/app/components/ATIAnalytics/canonical/sendBeaconOperaMiniScript/index.ts

Line 1 in 4d31a92

import isOperaProxy from '#app/lib/utilities/isOperaProxy';

[andrewscfc]

Might be worth calling out, if the inline JS that we create imports another file, I don't think the transpilation will work properly, so that needs to be bared in mind. There might be a way we can configure babel and webpack to include imported libraries directly inline maybe? But atm it seems like it can't, which was an issue found doing the opera mini script beacon

simorgh/src/app/components/ATIAnalytics/canonical/sendBeaconOperaMiniScript/index.ts

Line 1 in 4d31a92

import isOperaProxy from '#app/lib/utilities/isOperaProxy';

Should this:

import isOperaProxy from '#app/lib/utilities/isOperaProxy';

const sendBeaconOperaMiniScript = (atiPageViewUrlString: string) => `
    if (${isOperaProxy.toString()}() && !Boolean(window.hasOperaMiniScriptRan)) {
      window.hasOperaMiniScriptRan = true;

      var atiPageViewUrl = "${atiPageViewUrlString}";
      atiPageViewUrl += document.referrer ? "&ref=" + document.referrer : '';

      var xhr = new XMLHttpRequest();
      xhr.open("GET", atiPageViewUrl, true);
      xhr.withCredentials = true;
      xhr.send();
    }
`;

export default sendBeaconOperaMiniScript;

Be changed to something like this:

import isOperaProxy from '#app/lib/utilities/isOperaProxy';

const sendBeaconOperaMiniScript = (atiPageViewUrlString: string) => 
    if (isOperaProxy() && !Boolean(window.hasOperaMiniScriptRan)) {
      window.hasOperaMiniScriptRan = true;

      var atiPageViewUrl = "${atiPageViewUrlString}";
      atiPageViewUrl += document.referrer ? "&ref=" + document.referrer : '';

      var xhr = new XMLHttpRequest();
      xhr.open("GET", atiPageViewUrl, true);
      xhr.withCredentials = true;
      xhr.send();
    }
;

export default sendBeaconOperaMiniScript;

and the usage contain a tiny bit of inline js:

import sendBeaconOperaMiniScript from '#app/components/ATIAnalytics/canonical/sendBeaconOperaMiniScript';

...
        <script
          dangerouslySetInnerHTML={{
            __html: `(${sendBeaconOperaMiniScript.toString()})("${atiPageViewUrlString}")`,
          }}
        />
...

So we only use inline JS to pass parameters? This would apply the recommended pattern I think, can you check this @HarveyPeachey ?

[HarveyPeachey]

So we only use inline JS to pass parameters? This would apply the recommended pattern I think, can you check this @HarveyPeachey ?

The main problem is isOperaProxy():

import isOperaProxy from '#app/lib/utilities/isOperaProxy';

const sendBeaconOperaMiniScript = (atiPageViewUrlString: string) => 
    if (isOperaProxy() && !Boolean(window.hasOperaMiniScriptRan)) {

When transpiled, webpack attaches a reference to the imported module, it doesn't inline the imported code. Does that make sense? So the alternative is to take the code in isOperaProxy, and just paste it in manually.

[andrewscfc]

So we only use inline JS to pass parameters? This would apply the recommended pattern I think, can you check this @HarveyPeachey ?

The main problem is isOperaProxy():

import isOperaProxy from '#app/lib/utilities/isOperaProxy';

const sendBeaconOperaMiniScript = (atiPageViewUrlString: string) => 
    if (isOperaProxy() && !Boolean(window.hasOperaMiniScriptRan)) {

When transpiled, webpack attaches a reference to the imported module, it doesn't inline the imported code. Does that make sense? So the alternative is to take the code in isOperaProxy, and just paste it in manually.

Hmm, I can't think of an alternative to disallowing importing of scripts into inline JS? I think this might be better than the alternative of maintaining transpiled code in a string? What do you think @Isabella-Mitchell @karinathomasbbc @amoore108

[karinathomasbbc]

So we only use inline JS to pass parameters? This would apply the recommended pattern I think, can you check this @HarveyPeachey ?

The main problem is isOperaProxy():

import isOperaProxy from '#app/lib/utilities/isOperaProxy';

const sendBeaconOperaMiniScript = (atiPageViewUrlString: string) => 
    if (isOperaProxy() && !Boolean(window.hasOperaMiniScriptRan)) {

When transpiled, webpack attaches a reference to the imported module, it doesn't inline the imported code. Does that make sense? So the alternative is to take the code in isOperaProxy, and just paste it in manually.

Hmm, I can't think of an alternative to disallowing importing of scripts into inline JS? I think this might be better than the alternative of maintaining transpiled code in a string? What do you think @Isabella-Mitchell @karinathomasbbc @amoore108

I might need to do a bit more digging / thinking about this, but could we add some sort of annotation to files which are designed to be in-lined, and run some sort of unit test to ensure that it doesn't import code from elsewhere? (Not sure if this is even viable / possible).

[karinathomasbbc]

So we only use inline JS to pass parameters? This would apply the recommended pattern I think, can you check this @HarveyPeachey ?

The main problem is isOperaProxy():

import isOperaProxy from '#app/lib/utilities/isOperaProxy';

const sendBeaconOperaMiniScript = (atiPageViewUrlString: string) => 
    if (isOperaProxy() && !Boolean(window.hasOperaMiniScriptRan)) {

When transpiled, webpack attaches a reference to the imported module, it doesn't inline the imported code. Does that make sense? So the alternative is to take the code in isOperaProxy, and just paste it in manually.

Hmm, I can't think of an alternative to disallowing importing of scripts into inline JS? I think this might be better than the alternative of maintaining transpiled code in a string? What do you think @Isabella-Mitchell @karinathomasbbc @amoore108

I might need to do a bit more digging / thinking about this, but could we add some sort of annotation to files which are designed to be in-lined, and run some sort of unit test to ensure that it doesn't import code from elsewhere? (Not sure if this is even viable / possible).

There could be something here: https://eslint.org/docs/latest/rules/no-restricted-imports

[andrewscfc]

I might need to do a bit more digging / thinking about this, but could we add some sort of annotation to files which are designed to be in-lined, and run some sort of unit test to ensure that it doesn't import code from elsewhere? (Not sure if this is even viable / possible).

There could be something here: https://eslint.org/docs/latest/rules/no-restricted-imports

My question is more is disallowing imports sensible? I don't think we have to have tooling just some guidance we agree on in this case. I personally think it's better to have modern code with some moderate repetition rather than maintaining transpiled code as a string.