search

sveltejs / sapper

The next small thing in web development, powered by Svelte
https://sapper.svelte.dev
MIT License
7k stars 434 forks source link

Add defer to script tag created by rollup build #1722

Closed Evertt closed 3 years ago

Evertt commented 3 years ago

1123 added defer to the script tag when the bundler is not rollup. I think it was an oversight that it wasn't added to the other 2 lines.

Evertt commented 3 years ago

By the way, why does the rollup build get such a strange way of including the script tag?

benmccann commented 3 years ago

It has type=module which I believe implies defer

Evertt commented 3 years ago

It has type=module which I believe implies defer

Nope, it doesn't. Or at least the problem I'm running into is that if I have other script tags before %sapper.scripts% that use defer then they are not loaded in the right order. As in sapper's scripts are then loaded first, but I need them loaded last. And with the change in my PR it does work as expected.

edit

I'm reading about it now. I see that apparently it should infer defer, but I am clearly running into problems that imply that that's not true.

In more detail, I'm loading firebase from a CDN and using defer in those <script> tags. And in my sapper app code I make use of window.firebase. But I get the error that window.firebase is undefined, unless I apply the change in this PR.

So that to me implies that even though type=module is supposed to infer defer, it doesn't seem to honor the order of other deferred scripts.

benmccann commented 3 years ago

Hmm. MDN says "The defer attribute has no effect on module scripts — they defer by default.": https://developer.mozilla.org/en-US/docs/Web/HTML/Element/script

Evertt commented 3 years ago

Okay so it's one of those terrible bugs that sometimes pops up and sometimes doesn't. But I'd like to invite you to go to this URL: https://mytryout-246d2.web.app/

Open up your console and refresh a bunch of times without cache (cmd+shift+R on a mac). Sometimes you'll get an error related to firebase and/or firestore. That is because of scripts not being run in the right order. Do you get that too?

On my mac it happens both in Chrome and in Safari.

edit

After more experimenting I found out that my change doesn't actually improve this erroneous behavior. I just thought it did, because this bug doesn't appear every time you refresh.

Evertt commented 3 years ago

Here's some video evidence of the issue. I'm not sure what to do with it.

benmccann commented 3 years ago

There's a perhaps related issue in https://github.com/sveltejs/sapper/issues/1725, but it was suggested there that this PR doesn't fix that issue. Does adding the onload handler suggested there help you with your issue at all?

Evertt commented 3 years ago

@benmccann yes the onload handler does fix my issue. And yeah I was aware that my PR doesn't actually fix anything. It just seemed to, for a few minutes. 😅

edit

So uhm, would it maybe be a good idea to apply that to sveltekit too? To put the sveltekit starter in a window onload event?