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

fix(playground/runner): fix external script loading in playground #11017

Merged

Conversation

pseudopilot
Copy link
Contributor

@pseudopilot pseudopilot commented Apr 28, 2024

Summary

Fixes #10273

Problem

Using external scripts in the playground HTML section leads to the errors because the external script is loaded later than we start using entities from it in JAVASCRIPT section. As a result some code examples may not work like on this page:
https://developer.mozilla.org/en-US/docs/Games/Techniques/2D_collision_detection

Solution

When building the page (iframe) we remove the external scripts from document's body and put them in the document's head to make sure they are loaded before any other user scripts start running.

Alternative solution could be adding onload="onScriptLoaded()" to the external script and wrapping all the logic from JAVASCRIPT section into the function onScriptLoaded(). But this approach has its drawbacks.
First of all, everyone who writes some code snippets with the external scripts would need to know that this addition is needed and add it themselves.
Secondly, if the user wants to add few external scripts then the additional logic needs to be even more complicated.

Before

Screen.Recording.2024-04-27.at.18.30.10.mov
Screen.Recording.2024-04-28.at.10.26.21.mov

After

Screen.Recording.2024-04-27.at.18.29.21.mov
Screen.Recording.2024-04-28.at.10.23.30.mov

How did you test this change?

I couldn't test it on my localhost in dev mode because the runner.html is being fetched from the server. So I used the approach with overriding runner.html content in DevTools with the modified version.

@pseudopilot pseudopilot marked this pull request as ready for review April 28, 2024 09:04
@pseudopilot pseudopilot requested a review from a team as a code owner April 28, 2024 09:04
Copy link
Contributor

@caugner caugner left a comment

Choose a reason for hiding this comment

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

LGTM, just two nits.

@fiji-flo Any concerns with this change?

@pseudopilot pseudopilot force-pushed the fix-playground-external-script-loading branch from b3b98d2 to 4099a6b Compare May 3, 2024 12:58
Copy link
Contributor

@caugner caugner left a comment

Choose a reason for hiding this comment

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

Tiny nit, otherwise LGTM.

@pseudopilot
Copy link
Contributor Author

pseudopilot commented May 8, 2024

@fiji-flo , could you please find a moment to check the latest updates?

Copy link
Contributor

@fiji-flo fiji-flo left a comment

Choose a reason for hiding this comment

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

Thanks @pseudopilot. This looks great now.

@fiji-flo fiji-flo merged commit 8ba95b2 into mdn:main May 8, 2024
9 checks passed
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

Successfully merging this pull request may close these issues.

Live sample not working in '2D collision detection‘
3 participants