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

correctly handle the fact that nodeName is uppercase #251

Merged
merged 5 commits into from Dec 1, 2019
Merged

correctly handle the fact that nodeName is uppercase #251

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2ba86cc
correctly handle the fact that nodeName is uppercase
slimsag Dec 1, 2019
578efa2
workaround
slimsag Dec 1, 2019
e5fb70a
better comment
slimsag Dec 1, 2019
4555c49
fix typo
slimsag Dec 1, 2019
7ee19ae
fix typo
slimsag Dec 1, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion dom.go
View file
Open in desktop
Expand Up @@ -1225,7 +1225,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 := toLower(node.Get("nodeName").String())
if nextRender.tag != expectTag {
return ElementMismatchError{method: methodName, got: nextRender.tag, want: expectTag}
}
Expand Down
6 changes: 6 additions & 0 deletions dom_native.go
View file
Open in desktop
Expand Up @@ -2,6 +2,8 @@

package vecty

import "strings"

// Stubs for building Vecty under a native GOOS and GOARCH, so that Vecty
// type-checks, lints, auto-completes, and serves documentation under godoc.org
// as with any other normal Go package that is not under GOOS=js and
Expand Down Expand Up @@ -37,6 +39,10 @@ func RenderIntoNode(node SyscallJSValue, c Component) error {
return renderIntoNode("RenderIntoNode", node, c)
}

func toLower(s string) string {
return strings.ToLower(s)
}

var (
global jsObject
undefined wrappedObject
Expand Down
20 changes: 10 additions & 10 deletions dom_test.go
View file
Open in desktop
Expand Up @@ -563,7 +563,7 @@ func TestRerender_identical(t *testing.T) {

ts.ints.mock(`global.Call("requestAnimationFrame", func)`, 0)
ts.strings.mock(`global.Get("document").Get("readyState")`, "complete")
ts.strings.mock(`global.Get("document").Call("querySelector", "body").Get("nodeName")`, "body")
ts.strings.mock(`global.Get("document").Call("querySelector", "body").Get("nodeName")`, "BODY")
ts.truthies.mock(`global.Get("document").Call("querySelector", "body")`, true)

// Perform the initial render of the component.
Expand Down Expand Up @@ -652,7 +652,7 @@ func TestRerender_change(t *testing.T) {

ts.ints.mock(`global.Call("requestAnimationFrame", func)`, 0)
ts.strings.mock(`global.Get("document").Get("readyState")`, "complete")
ts.strings.mock(`global.Get("document").Call("querySelector", "body").Get("nodeName")`, "body")
ts.strings.mock(`global.Get("document").Call("querySelector", "body").Get("nodeName")`, "BODY")
ts.truthies.mock(`global.Get("document").Call("querySelector", "body")`, true)

// Perform the initial render of the component.
Expand Down Expand Up @@ -757,7 +757,7 @@ func TestRerender_Nested(t *testing.T) {

ts.ints.mock(`global.Call("requestAnimationFrame", func)`, 0)
ts.strings.mock(`global.Get("document").Get("readyState")`, "complete")
ts.strings.mock(`global.Get("document").Call("querySelector", "body").Get("nodeName")`, "body")
ts.strings.mock(`global.Get("document").Call("querySelector", "body").Get("nodeName")`, "BODY")
ts.truthies.mock(`global.Get("document").Call("querySelector", "body")`, true)

// Perform the initial render of the component.
Expand Down Expand Up @@ -857,7 +857,7 @@ func TestRerender_persistent(t *testing.T) {

ts.ints.mock(`global.Call("requestAnimationFrame", func)`, 0)
ts.strings.mock(`global.Get("document").Get("readyState")`, "complete")
ts.strings.mock(`global.Get("document").Call("querySelector", "body").Get("nodeName")`, "body")
ts.strings.mock(`global.Get("document").Call("querySelector", "body").Get("nodeName")`, "BODY")
ts.truthies.mock(`global.Get("document").Call("querySelector", "body")`, true)

lastRenderedComponent = nil
Expand Down Expand Up @@ -921,7 +921,7 @@ func TestRerender_persistent_direct(t *testing.T) {

ts.ints.mock(`global.Call("requestAnimationFrame", func)`, 0)
ts.strings.mock(`global.Get("document").Get("readyState")`, "complete")
ts.strings.mock(`global.Get("document").Call("querySelector", "body").Get("nodeName")`, "body")
ts.strings.mock(`global.Get("document").Call("querySelector", "body").Get("nodeName")`, "BODY")
ts.truthies.mock(`global.Get("document").Call("querySelector", "body")`, true)

lastRenderedComponent = nil
Expand Down Expand Up @@ -987,7 +987,7 @@ func TestRenderBody_ExpectsBody(t *testing.T) {
ts := testSuite(t)
defer ts.done()

ts.strings.mock(`global.Get("document").Call("querySelector", "body").Get("nodeName")`, "body")
ts.strings.mock(`global.Get("document").Call("querySelector", "body").Get("nodeName")`, "BODY")
ts.truthies.mock(`global.Get("document").Call("querySelector", "body")`, true)

var gotPanic string
Expand Down Expand Up @@ -1048,7 +1048,7 @@ func TestRenderBody_Standard_loaded(t *testing.T) {

ts.strings.mock(`global.Get("document").Get("readyState")`, "loaded")
ts.ints.mock(`global.Call("requestAnimationFrame", func)`, 0)
ts.strings.mock(`global.Get("document").Call("querySelector", "body").Get("nodeName")`, "body")
ts.strings.mock(`global.Get("document").Call("querySelector", "body").Get("nodeName")`, "BODY")
ts.truthies.mock(`global.Get("document").Call("querySelector", "body")`, true)

RenderBody(&componentFunc{
Expand All @@ -1067,7 +1067,7 @@ func TestRenderBody_Standard_loading(t *testing.T) {

ts.strings.mock(`global.Get("document").Get("readyState")`, "loading")
ts.ints.mock(`global.Call("requestAnimationFrame", func)`, 0)
ts.strings.mock(`global.Get("document").Call("querySelector", "body").Get("nodeName")`, "body")
ts.strings.mock(`global.Get("document").Call("querySelector", "body").Get("nodeName")`, "BODY")
ts.truthies.mock(`global.Get("document").Call("querySelector", "body")`, true)

RenderBody(&componentFunc{
Expand All @@ -1088,7 +1088,7 @@ func TestRenderBody_Nested(t *testing.T) {

ts.strings.mock(`global.Get("document").Get("readyState")`, "complete")
ts.ints.mock(`global.Call("requestAnimationFrame", func)`, 0)
ts.strings.mock(`global.Get("document").Call("querySelector", "body").Get("nodeName")`, "body")
ts.strings.mock(`global.Get("document").Call("querySelector", "body").Get("nodeName")`, "BODY")
ts.truthies.mock(`global.Get("document").Call("querySelector", "body")`, true)

RenderBody(&componentFunc{
Expand Down Expand Up @@ -1130,7 +1130,7 @@ func TestKeyedChild_DifferentType(t *testing.T) {

ts.ints.mock(`global.Call("requestAnimationFrame", func)`, 0)
ts.strings.mock(`global.Get("document").Get("readyState")`, "complete")
ts.strings.mock(`global.Get("document").Call("querySelector", "body").Get("nodeName")`, "body")
ts.strings.mock(`global.Get("document").Call("querySelector", "body").Get("nodeName")`, "BODY")
ts.truthies.mock(`global.Get("document").Call("querySelector", "body")`, true)

comp := &componentFunc{
Expand Down
7 changes: 7 additions & 0 deletions dom_wasmjs_gopherjs.go
View file
Open in desktop
Expand Up @@ -30,6 +30,13 @@ func RenderIntoNode(node js.Value, c Component) error {
return renderIntoNode("RenderIntoNode", wrapObject(node), c)
}

func toLower(s string) string {
// We must call the prototype method here to workaround a limitation of
// syscall/js in both Go and GopherJS where we cannot call the
// `toLowerCase` string method. See https://github.com/golang/go/issues/35917
return js.Global().Get("String").Get("prototype").Get("toLowerCase").Call("call", js.ValueOf(s)).String()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have a way of checking whether performance is impacted doing this on each toLower call, compared to:

func toLower(s string) string {
    return toLowerCase.Call("call", js.ValueOf(s)).String()
}

var toLowerCase = js.Global().Get("String").Get("prototype").Get("toLowerCase")

Also, the string -> js.Value -> string conversions can be short-circuited if you're willing to create a more dedicated helper for getting the node name, for example:

// +build js
func lowerNodeName(node jsObject) string {
    return toLowerCase.Call("call", node.Get("nodeName")).String()
}

// +build !js
func lowerNodeName(node jsObject) string {
    return strings.ToLower(node.Get("nodeName").String())
}

I don't have a sense whether the performance difference would be significant or negligible.

If performance isn't a bottneck or priority for now, this can all be considered later of course.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think even if performance of this is poor, it would probably be negligible AND since it only occurs on calls to RenderBody, RenderInto, RenderIntoNode it is not very frequently called, so as-is is probably fine for now.

Benchmarking things overall (all of Vecty) would definitely be a good idea, though that will probably come a bit later.

}

var (
global = wrapObject(js.Global())
undefined = wrappedObject{js.Undefined()}
Expand Down