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

WIP 5944 Fix race condition: is Svelte generated code for on:load listener in wrong order? #5944

Closed
t829702 opened this issue Jan 31, 2021 · 10 comments

Comments

@t829702
Copy link

t829702 commented Jan 31, 2021

there are many reasons to load an external js script (like stripe, mapbox, gpt tag, ... scripts), rather than bundle it with one's own code,

https://www.nielsvandermolen.com/external-javascript-sveltejs/

something like this indeed works in Svelte REPL but seems doing nothing in Sapper, I copied this sample code https://svelte.dev/repl/28f4b2e36e4244b8b23cae3d584c4c88?version=3.16.6 to an exactly same page in Sapper, the on:load={initializeRemarkable} is not called

<script src="https://cdnjs.cloudflare.com/ajax/libs/remarkable/2.0.0/remarkable.min.js" on:load={initializeRemarkable}></script>
@t829702
Copy link
Author

t829702 commented Jan 31, 2021

By looking at generated code in above REPL example at where it append the external script to document.head, I suspect this is an Svelte issue rather than only Sapper issue;

    m: async function mount(target, anchor) {	// changed to async for the wait-a-sec
    	append_dev(document.head, script);

	// if add wait a sec here (to simulate some delay) before attach the 'load' event listener,
	//   then the script is almost sure finished loading and the initializeRemarkable listener will NOT get called
	await new Promise(resolve => setTimeout(resolve, 1000));

    	// ... omitted many lines here

    	if (!mounted) {
    		dispose = [
    			listen_dev(script, "load", /*initializeRemarkable*/ ctx[2], false, false, false),
    			listen_dev(textarea, "input", /*textarea_input_handler*/ ctx[3])
    		];
   		mounted = true;
    	}
    },

compare with a normal vanilla js of loading an external js tag on the fly (below), it needs to attach the onload listener before append to the DOM; if not this order, then it's.a racing issue, there might be a delay between and then that link might be loaded very fast before the onload listener attached, then the onload listener will NOT get called;

const script = document.createElement('script');
script.scr = 'https://cdnjs.cloudflare.com/ajax/libs/remarkable/2.0.0/remarkable.min.js';
script.onload = function() { ... }	// or .attachEventListener('load', function() { ... })
document.head.appendChild(script);

So I believe this Svelte generated code for on:load needs to change order to before append_dev(document.head, script); ?

@t829702 t829702 changed the title want an official documentation in examples of loading external Javascript libraries want an official documentation in examples of loading external Javascript libraries; is Svelte generated code for on:load listener in wrong order? Jan 31, 2021
@t829702
Copy link
Author

t829702 commented Feb 5, 2021

@Conduitry this isn't only a question but can also be a bug report? I believe the attaching 'load' event too late after mounted to the DOM could be root cause of the problem

@Phaqui
Copy link
Contributor

Phaqui commented Feb 5, 2021

Technically speaking, I'd say it's a bug, yes.

It may be very unlikely to happen, given that the time it takes to request the source script, is probably a lot longer than the time it takes to reach the point in the mount function where the 'load' handler is attached... but then again, what do I know about the browser's and scripts intricacies. If we insist on not marking this is a bug, we should at least be very, very certain that this racy bug is extremely unlikely to happen - but it's probably a lot easier to just fix the code generation to correctly add the event listener before appending the script to the DOM.

@t829702
Copy link
Author

t829702 commented Feb 14, 2021

I believe #5988 may be a similar race condition of load event on external resources?

@t829702 t829702 changed the title want an official documentation in examples of loading external Javascript libraries; is Svelte generated code for on:load listener in wrong order? WIP 5944 Fix race condition: is Svelte generated code for on:load listener in wrong order? Feb 14, 2021
@t829702
Copy link
Author

t829702 commented Feb 18, 2021

it's weird seems no one from core maintainer want to have a look? even with PR #5968 available from @Phaqui (Thanks to @Phaqui !)
/cc @antony who responded on #5988 which makes me believe there are multiple race condition problems exist in Svelte

@antony
Copy link
Member

antony commented Feb 18, 2021

@t829702 I'm not sure what your notion of the Svelte SLA is, but far from not wanting to have a look, we have a lot on our plate as well as full time jobs, and we simply haven't got around to looking at this yet. Feel free to do your own investigation and come up with a solution for it if there is an issue, but don't expect us to be able to jump on every single issue immediately.

Skimming the issue briefly, I'm going to say a delay in script load and execution has nothing to do with Svelte nor is it an issue, it's just the nature of Javascript and the way browsers work. This is the specific reason I created https://github.com/beyonk-adventures/async-script-loader - which is based off something I found in React, as a workaround for the exact same issue there.

You can't guarantee the load execution time of scripts without holding up the page load, which is bad - so you simply trigger your execution based on when a script is "ready". Have a look.

@t829702
Copy link
Author

t829702 commented Mar 25, 2021

based on when a script is "ready".

script.onload = function() { ... }	// or .attachEventListener('load', function() { ... })

@stale
Copy link

stale bot commented Jun 26, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale-bot label Jun 26, 2021
@stale stale bot removed the stale-bot label Jun 26, 2021
@stale stale bot removed the stale-bot label Jun 27, 2021
@stale
Copy link

stale bot commented Dec 24, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@dummdidumm
Copy link
Member

Closing in favor of #11046

@dummdidumm dummdidumm closed this as not planned Won't fix, can't repro, duplicate, stale Apr 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants