Fix script loading issue in Firefox fixes #601

[jchatard]

In Firefox the onload attribute is not always triggered on script element so we instead use script.addEventListener('load')

[changeset-bot]

⚠️ No Changeset found

Latest commit: 9e98a7b

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[atk]

This loses us the ability to use [handler, argument] event props. We also seem to have some issues with the calling of spread() that need to be addressed. I would probably go and replace it. In the meantime, can you try on:load as a workaround?

[jchatard]

I'm not sure I fully understand what you suggest with on:load.

My understanding is that on: is for JSX markup.

But neither in this PR or in my user land code, do I have JSX involved.

My user land code:

  createScriptLoader({
    src: "/assets/glide.min.js",
    async: true,
    onLoad() {
      setHasGlideLoaded!(true);
    },
  });

What do you suggest exactly?

[atk]

Use 'on:load': () => ....

[atk]

I just checked and 'on:load' seems to work flawlessly, so we can pull off the following trick:

const OMITTED_PROPS = ["src", "onLoad", "onload"] as const;
//...
  const script = document.createElement("script");
  const [local, scriptProps] = splitProps(props, OMITTED_PROPS);
  const onload = local.onload || local.onLoad ? { 'on:load': local.onload || local.onLoad } : {};
  setTimeout(() => spread(script, mergeProps(scriptProps, onload), false, true));

[atk]

But I'm afraid onError might have the same issue?

[jchatard]

I hope to have time to test this today.

[jchatard]

Hi @atk sorry for the delay, couldn't find a time slot to check this before.

I just tried what you suggested, but this doesn't seem to solve the issue.

In the repro I created, I changed the scriptLoader to:

createScriptLoader({
  src: "https://cdn.jsdelivr.net/npm/jquery@3.7.1/dist/jquery.min.js",
  'on:load': () => {
    console.log("jQuery loaded");
    setJqueryLoaded(true);
  },
});

But when running tests, it fails the same way as before.

[atk]

I just checked, on:load is transpiled by JSX expressions, so that does not help, we need to set the events ourselves. Let's use the actual events, but for all events. I'll create a PR.

[atk]

Closed; being fixed in #637