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.

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

Add support for rendering into non-body DOM nodes #249

Merged
merged 4 commits into from Dec 1, 2019
Merged

Conversation

@slimsag
Copy link
Member

slimsag commented Dec 1, 2019

This PR adds support for rendering into arbitrary DOM nodes in the same way that rendering into the body worked previously, with one exception: blocking.

RenderBody remains blocking, since it effectively becomes a helper that 90% of Vecty applications will rely on, whereas RenderInto and RenderIntoNode which are introduced in this PR do not block. This is because:

a. Anyone rendering Vecty into arbitrary DOM nodes will likely want to do it more than once, so making the API of these functions non-blocking makes sense.
b. Anyone using these functions will be a more advanced user and understand when blocking is / is not needed.

In order to land this PR earlier for users who need this functionality, tests will not be added for the new functionality. They will be added in a follow-up as part of #168.

Fixes #81
Fixes #247

@slimsag slimsag mentioned this pull request Dec 1, 2019
0 of 3 tasks complete
This PR adds support for rendering into arbitrary DOM nodes in the same way
that rendering into the `body` worked previously, with one exception: blocking.

`RenderBody` remains blocking, since it effectively becomes a helper that 90%
of Vecty applications will rely on, whereas `RenderInto` and `RenderIntoNode`
which are introduced in this PR do not block. This is because:

a. Anyone rendering Vecty into arbitrary DOM nodes will likely want to do it
   more than once, so making the API of these functions non-blocking makes
   sense.
b. Anyone using these functions will be a more advanced user and understand
   when blocking is / is not needed.

In order to land this PR earlier for users who need this functionality, tests
will not be added. They will be added in a follow-up as part of #168.

Fixes #81
Fixes #247
@slimsag slimsag force-pushed the sg/render-into branch from 40d731e to 5066a6f Dec 1, 2019
@slimsag slimsag merged commit acd5697 into master Dec 1, 2019
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@slimsag slimsag deleted the sg/render-into branch Dec 1, 2019
}
if nextRender.tag != "body" {
panic("vecty: RenderBody expected Component.Render to return a body tag, found \"" + nextRender.tag + "\"")
expectTag := node.Get("nodeName").String()

This comment has been minimized.

Copy link
@dmitshur

dmitshur Dec 1, 2019

Member

I believe I'm running into an issue with nodeName case sensitivity here (see https://developer.mozilla.org/en-US/docs/Web/API/Node/nodeName and https://johnresig.com/blog/nodename-case-sensitivity/).

My program is doing:

// Initial frontend render.
err := vecty.RenderInto("body", body)
if err != nil {
	panic(fmt.Errorf("internal error: unexpected error from vecty.RenderInto: %v", err))
}

And getting:

Uncaught Error: internal error: unexpected error from vecty.RenderInto: vecty: RenderInto: expected Component.Render to return a "BODY", found "body"

I'm using HTML, Chrome 78.0.3904.108 on macOS.

This comment has been minimized.

Copy link
@dmitshur

dmitshur Dec 1, 2019

Member

I tried the following quick change to see if it would be sufficient to solve the problem:

diff --git a/dom.go b/dom.go
index 0eb000e..164a5a3 100644
--- a/dom.go
+++ b/dom.go
@@ -2,6 +2,7 @@ package vecty
 
 import (
 	"reflect"
+	"strings"
 )
 
 // batch renderer singleton
@@ -1225,7 +1226,7 @@ func renderIntoNode(methodName string, node jsObject, c Component) error {
 	if skip {
 		panic("vecty: " + methodName + ": Component.SkipRender illegally returned true")
 	}
-	expectTag := node.Get("nodeName").String()
+	expectTag := strings.ToLower(node.Get("nodeName").String())
 	if nextRender.tag != expectTag {
 		return ElementMismatchError{method: methodName, got: nextRender.tag, want: expectTag}
 	}

It was; after that, Go Package Store was rendering the frontend without issues.

I'm not sure if that's the optimal long term fix, or if there's a better way. You may not want to import a new strings package just for one function.

I tried using JavaScript's toLowerCase method, but for some reason that didn't work in the context of Vecty:

Uncaught Error: syscall/js: call of Value.Call on string

Even though it seems to work fine elsewhere, e.g., see https://gopherjs.github.io/playground/#/UxD9vDjbf0.

Hope this is helpful.

This comment has been minimized.

Copy link
@dmitshur

dmitshur Dec 1, 2019

Member

About the "syscall/js: call of Value.Call on string" error, I found it happens when using syscall/js package with GopherJS but not when using github.com/gopherjs/gopherjs/js package. So it's not Vecty-specific beyond Vecty using syscall/js.

It happens when using syscall/js with both GopherJS and Go/WebAssembly, so I filed golang/go#35917 to learn more about that.

This comment has been minimized.

Copy link
@slimsag

slimsag Dec 1, 2019

Author Member

Thank you for investigating this! I've sent #251 to fix the issue on the Vecty side

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.