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.

Sign up for GitHub

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

Jump to bottom

Create the default root element and initial render after the document is ready #64

Closed
wants to merge 1 commit into from
Closed

Create the default root element and initial render after the document is ready #64

wants to merge 1 commit into from

Conversation

tzellman
Copy link
Contributor

@tzellman tzellman commented Feb 8, 2017

It seems safest to wait until the document is ready before we attempt to:

  • create the default root element
  • initially call render

@codecov
Copy link

codecov bot commented Feb 8, 2017 •
edited
Loading

Codecov Report

Merging #64 into master will increase coverage by 0.81%.

@@            Coverage Diff             @@
##           master      #64      +/-   ##
==========================================
+ Coverage   40.55%   41.37%   +0.81%     
==========================================
  Files           3        3              
  Lines         143      145       +2     
==========================================
+ Hits           58       60       +2     
  Misses         85       85
Impacted Files Coverage Δ
app.js 33.59% <80%> (+1.05%) ✅

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4126833...29ca864. Read the comment docs.

@jorgebucaran jorgebucaran self-requested a review February 8, 2017 17:34
@tzellman
Copy link
Contributor Author

tzellman commented Feb 9, 2017

I might need to make a slight mod to rebase with the changes made in 4126833.

@jorgebucaran
Copy link
Owner

@tzellman It seems safest to wait until the document is ready before we attempt to:

Playing devil's advocate here: why is it safest? and also, how come we were not getting any errors until now? That could be indicating a deeper problem.

@FlorianWendelborn
Copy link

@jbucaran If that's helpful I definitely already had that issue in 0.0.7. Took a while to understand while the subs didn't work, but made sense that the event never fired when executing app on load.

@jorgebucaran
Copy link
Owner

@dodekeract Minor correction, it's subscriptions now.

@jorgebucaran jorgebucaran mentioned this pull request Feb 9, 2017
Closed
2 tasks
@tzellman
Copy link
Contributor Author

tzellman commented Feb 9, 2017

The reason I made the change was strictly due to potential timing issues. If your JS runs in the <head> tag, then it is possible to attempt to create the default root element before the DOM is ready. Same goes for the render method, in case you passed in a truthy value for root.

@jorgebucaran
Copy link
Owner

jorgebucaran commented Feb 9, 2017 •
edited
Loading

@tzellman Exactly what we're discussing here: #64 #69

@FlorianWendelborn
Copy link

@jbucaran You mean #69. 🙂

@jorgebucaran
Copy link
Owner

Yup. 😅

@tzellman
Copy link
Contributor Author

tzellman commented Feb 9, 2017

Just posted a comment with example in #69.

@jorgebucaran
Copy link
Owner

jorgebucaran commented Feb 9, 2017 •
edited
Loading

What do other frameworks usually do here? Does anybody know?

@FlorianWendelborn
Copy link

FlorianWendelborn commented Feb 9, 2017 •
edited
Loading

@jbucaran React definitely doesn't work/do this, because you have to manually give ReactDOM.render() a DOMElement. I guess it's a bit more low-level than hyperapp in that regard.

Now that we're talking about that - is it supported to render directly to <body> in hyperapp? That's another thing ReactDOM doesn't like.

@jorgebucaran
Copy link
Owner

@dodekeract Try app({ root: document.body }).

@jorgebucaran
Copy link
Owner

Root elements, subscriptions and initial render (incl. router support) landed in master.

Thank you @tzellman for bringing this up!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FlorianWendelborn FlorianWendelborn FlorianWendelborn approved these changes

@jorgebucaran jorgebucaran Awaiting requested review from jorgebucaran

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tzellman @jorgebucaran @FlorianWendelborn