Skip to content
This repository was archived by the owner on Jan 11, 2023. It is now read-only.

Add defer to script tag created by rollup build #1722

Closed
wants to merge 1 commit into from

Conversation

Evertt
Copy link

@Evertt Evertt commented Feb 17, 2021

#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
Copy link
Author

Evertt commented Feb 17, 2021

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

@benmccann
Copy link
Member

It has type=module which I believe implies defer

@Evertt
Copy link
Author

Evertt commented Feb 17, 2021

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
Copy link
Member

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
Copy link
Author

Evertt commented Feb 17, 2021

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
Copy link
Author

Evertt commented Feb 17, 2021

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

@benmccann
Copy link
Member

There's a perhaps related issue in #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
Copy link
Author

Evertt commented Mar 31, 2021

@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?

@Evertt Evertt closed this Mar 31, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants