WS-2095: Updates video plugins to use Var

[Isabella-Mitchell]

Resolves JIRA: WS-2095

Summary

Fix to try resolve safari iphone issue (slack)

Code changes

• List key code changes that have been made.

Testing

1. Tested this on my feature branch on preview (using a combination of cache busting and local overrides). It seems to work as desired.

Useful Links

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

[Copilot]

Pull request overview

Renames the public SMP video overlay plugin entrypoint function, likely as part of addressing an iPhone Safari playback/overlay issue.

Changes:

• Renamed the plugin initializer from runPlugin to runVideoOverlayPlugin.
• Updated the CommonJS export to export runVideoOverlayPlugin instead of runPlugin.

[amoore108]

For this line

simorgh/public/smpPlugins/video-overlay-plugin.js

Line 36 in ed1264c

const runVideoOverlayPlugin = (utils, data) => {

and the same in the fullscreen plugin, we could try changing it to var runPlugin instead of const runPlugin.

That should let you re-declare the same value, but not sure on how it will actually behave in reality.

[Isabella-Mitchell]

For this line

simorgh/public/smpPlugins/video-overlay-plugin.js

Line 36 in ed1264c

const runVideoOverlayPlugin = (utils, data) => {

and the same in the fullscreen plugin, we could try changing it to var runPlugin instead of const runPlugin.
That should let you re-declare the same value, but not sure on how it will actually behave in reality.

Good idea. I've actually updated it based on some copilot advice, suggesting the issue was being caused by the export provided for unit tests (that import this plugin in a Node/CommonJS environment). I don't actually think we're using this and just copied it from SMP.

[amoore108]

Good idea. I've actually updated it based on some copilot advice, suggesting the issue was being caused by the export provided for unit tests (that import this plugin in a Node/CommonJS environment). I don't actually think we're using this and just copied it from SMP.

I think it'll need exported to some degree though as otherwise there is no function that can be executed by SMP?

[Isabella-Mitchell]

Good idea. I've actually updated it based on some copilot advice, suggesting the issue was being caused by the export provided for unit tests (that import this plugin in a Node/CommonJS environment). I don't actually think we're using this and just copied it from SMP.

I think it'll need exported to some degree though as otherwise there is no function that can be executed by SMP?

Ahhh ok, I was following public/smpPlugins/fullscreen.js - but I can see this is used in a different way. I've updated to include an export. Does this look ok? Copilot says:

Yes, your current approach will work for both SMP and Node environments:

• The global runPlugin function is declared, so SMP can execute it as expected when loading the plugin in the browser.
• The CommonJS export (module.exports = { runPlugin }) is only active in Node environments, so it does not affect the browser global scope or cause duplicate variable errors.
• The function is not redeclared, so you avoid the Safari duplicate variable issue.

This pattern is standard for SMP plugins and will not cause any issues. SMP will find and execute runPlugin in the browser, and your tests or Node scripts can import it via module.exports. You are safe to proceed with this implementation.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.

[amoore108]

Ahhh ok, I was following public/smpPlugins/fullscreen.js - but I can see this is used in a different way. I've updated to include an export. Does this look ok? Copilot says:

Ah sorry! I thought for sure we did the same thing in fullscreen.js but we don't! The way you had it may be ok.

I think it will still trip up though if its left as const instead of var though, unless these get transpiled and are given different variable names?

[Isabella-Mitchell]

Ahhh ok, I was following public/smpPlugins/fullscreen.js - but I can see this is used in a different way. I've updated to include an export. Does this look ok? Copilot says:

Ah sorry! I thought for sure we did the same thing in fullscreen.js but we don't! The way you had it may be ok.

I think it will still trip up though if its left as const instead of var though, unless these get transpiled and are given different variable names?

Cool, I have followed the approach in the PS plugin to use var and added some eslint-disable comments.

[amoore108]

Cool, I have followed the approach in the PS plugin to use var and added some eslint-disable comments.

Thanks 👍 Do you think we should update the fullscreen.js one as well to use var? Just to ensure there isn't a conflict between const and var across these.

[HarveyPeachey]

Any reason why we're using var instead of let?

[amoore108]

Any reason why we're using var instead of let?

let will have the same behaviour as const for a variable with the same name:

let x = 10;
let x = 20; // ❌ SyntaxError: Identifier 'x' has already been declared

let can be reassigned, but not re-declared.

[Isabella-Mitchell]

Cool, I have followed the approach in the PS plugin to use var and added some eslint-disable comments.

Thanks 👍 Do you think we should update the fullscreen.js one as well to use var? Just to ensure there isn't a conflict between const and var across these.

Yep makes sense - heads up I have got these changes temporarily going onto preview via this PR so hopefully I can check they work.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.

[Copilot]

The fullscreen.js plugin is missing the same module.exports pattern that was added to video-overlay-plugin.js. Based on the comment in video-overlay-plugin.js about exporting for tests, this plugin should also have a similar export mechanism if it needs to be tested.

Currently, fullscreen.js declares runPlugin but never exports it, which means it can only be used when loaded as a script tag by SMP and cannot be imported or tested in a Node.js environment. Consider adding the same typeof check and export pattern for consistency.

Suggested change

	};
	};
	if (typeof module !== 'undefined' && typeof module.exports !== 'undefined') {
	module.exports = runPlugin;
	}

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.