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.

[karinathomasbbc]

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.

I agree that modern code (even if there is repetition) is definitely more desirable than maintaining transpiled code as a string. It means we can much more easily test it.

I would just want to ensure that there's a "safe" way of making changes such that tests will catch any errors, and without the need for a lot of manual regression testing. Rather than devs or those doing the code review having to remember to do something.

I know I may be solutionising a bit, but will we need to do some sort of either snapshot testing, or verification that the contents of these transpiled-code-in-a-string files will work once the application is running? e.g. we have the ATI Analytics E2E tests which check that the correct beacons are being fired & the URL constructed properly - do we need to add these types of tests too?

[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.

The good thing about this is that if we take the approach outlined by Andrew, and we try to use toString() on a function with imports (per #12592 (comment)), then we get a runtime error when we run yarn dev. Would that be a good enough indication that something is broken & needs to be fixed?

[karinathomasbbc]

I've also spun up a quick PR to demonstrate what would happen if we tried to convert a stringified function to a normal function which imports a file: #12656

Thankfully, lotttttts of tests fail, which would mean that devs would need to figure out & fix the problem in order to be able to merge! So we don't need to worry about adding any form of automation, as our PR checks would catch it 🎉