-
Notifications
You must be signed in to change notification settings - Fork 13
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
[DEV-2184] [@prezly/slate-renderer] Prevent the same embed scripts being loaded multiple times #18
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good but I don't know how to reproduce the issue, therefore I don't know what is the source of the issue, therefore I could not test this PR.
Please make sure to test it before merging.
In case of any issues - please, let me know :)
@@ -13,12 +13,20 @@ const injectOembedMarkup = ({ oembed, onError, target }: Parameters): void => { | |||
container.innerHTML = oembed.html || ''; | |||
const embedScripts = Array.from(container.getElementsByTagName('script')); | |||
|
|||
const currentScriptSources = Array.from(document.getElementsByTagName('script')).map((script) => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this comment is in an unrelated place
https://linear.app/prezly/issue/DEV-2184#comment-826b8e56
Though I couldn't find a way to test that it indeed works. I believe I will have to wait for @kamilmielnik to return and help with that
Yes, there is no easy way to test it. I've created an issue for it: #21
What you can do now is:
- Set up the
slate
repository locally - Make sure to build it
- Use local path (
file:
) inpackage.json
of your project to specify path to@prezly/slate-renderer
package you've built locally - see npm docs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for a tip, will try to test it like that!
copyScriptAttributes(embedScript, script); | ||
script.addEventListener('error', onError); | ||
// Prevent same scripts from loading multiple times | ||
if (!currentScriptSources.includes(embedScript.getAttribute('src'))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌
So I have to admit to a terrible overlook from my side... I realized it only after reading your comment in linear 🤦♂️ .
Please create an html file with the following content and open it in your browser. Then open up developer tools, refresh the page and answer this question: how many times has https://cdn.jsdelivr.net/lodash/4/lodash.min.js
been loaded?
<!DOCTYPE html>
<html>
<head>
<script src="https://cdn.jsdelivr.net/lodash/4/lodash.min.js"></script>
<script src="https://cdn.jsdelivr.net/lodash/4/lodash.min.js"></script>
</head>
<body>
<script src="https://cdn.jsdelivr.net/lodash/4/lodash.min.js"></script>
<script src="https://cdn.jsdelivr.net/lodash/4/lodash.min.js"></script>
</body>
</html>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it's loaded only one time 🤔
So the I guess this PR can be safely discarded :)
Attempted fix for this issue:
https://linear.app/prezly/issue/DEV-2184/errors-in-console-when-browsing-a-page-with-multiple-videos