-
Notifications
You must be signed in to change notification settings - Fork 780
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
Should HyperApp Wait For The DOM Before Rendering? #69
Comments
Also related #64. |
I'd prefer to keep it out for now. It's good to be explicit, and the majority have no problems with the way it is now. However, that being said I see both sides and I don't really mind if it gets included. |
@terkelg Yes, since the It might also be worth it to investigate if |
Just to show how this fails when attempting to use a script in the I always attach scripts in the body, so I never see this issue, but #64 seems like a minimal change in order to support also including scripts in the head, which some users may do. I also believe in the fail-fast mentality, so at the very least it might be worth noting in the docs that users should include scripts just before you need it, and no sooner, i.e. in the |
Why would they do that? Perhaps you can elaborate on this @dodekeract, since you were who first raised this concern in #61. 🙏 |
Yeah, I fully hear you on that. SoC is an idiom I fully subscribe to. I heavily use DI, so often it is assumed that you aren't allowed to start using something before it is properly initialized (e.g. via constructor/setter injection). I think you won't see the issue in the first place since most users will properly initialize by ordering their scripts in the The main reason I made the PR was because the change seemed very minimal and would actually not have any affect on current usage, plus it would support the non-ideal behavior of including scripts in the |
@jbucaran Well, in my case I'm currently writing the Video SDK for @attach-live. It has to be easily embedded by as many third-party developers as possible. Hence I can't force the position where the It also may not be possible to add a In addition to that I actually really dislike putting I guess the reason why I don't like it is that CSS is embedded into the head too and I personally think that the body is for HTML, not JavaScript. I know that doesn't work with |
No matter what is the use case, it is absolutely user job to include the app wherever he want. It is just enough to not crash and to work the same way. Please no magics. It should be just documented that in case you want to include it in So 👎 |
If you look at the change in #64 then you'll see there is no magic. |
@tunnckoCore I value your opinion, but I need to make sure you understand what @tzellman is proposing. The idea is to only call the first render once the DOM has loaded. So, I'm not sure if it's really magic anymore. I'm still not sure though. In the future, I'd like to support Electron, so it will be necessary to decouple the |
@jbucaran electron would be awesome. I think it actually has |
@jbucaran thanks! :) My "please no magic" was referred to the first post. As about #64 I didn't follow the discussion there, just reviewed the code for a sec. Don't know. I'm not sure. I'm going to scan the #64 :) edit: i like that question
|
This is implemented since 0.3.0 or 0.4.0, can't remember. |
If you only want to make your vote count please just 👍 or 👎 this comment. I added both so it's easy to click.
Things that need to be done
window.addEventListener('load')
should be implicitThe Problem
When including
app()
in a script in<head>
hyperapp will crash because it tries to append itself onto the not-yet-existing DOM.Note: Some might suggest to just include the script at the bottom of the page, however - in some use-cases (like mine) that's not possible. It also feels like a workaround IMHO.
This can be solved in (at least) 2 ways:
Solution 1
Force users to add
window.addEventListener('load', () => app(...))
to their scripts.Pro
Contra
Solution 2
Implicitly do the same as in solution 1.
Pro
Contra
Might be barely considered "Magic". Should be properly documented. However I don't see any use-case where hyperapp is supposed to crash. 😄
Meta:
Follow-up issue
Initial issue:
subs
didn't work when ran byonload
The text was updated successfully, but these errors were encountered: