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

correctly handle the fact that nodeName is uppercase #251

Merged
merged 5 commits into from Dec 1, 2019
Merged

Conversation

@slimsag
Copy link
Member

slimsag commented Dec 1, 2019

@slimsag

This comment has been minimized.

Copy link
Member Author

slimsag commented Dec 1, 2019

@dmitshur thank you for reporting! I missed the fact that nodeName was uppercase. Would you be able to confirm if this fix works for you?

@slimsag

This comment has been minimized.

Copy link
Member Author

slimsag commented Dec 1, 2019

panic: syscall/js: call of Value.Call on string

Ah, this actually appears to be an incorrect limitation of GOOS=js GOARCH=wasm in at least Go 1.12 and perhaps also in master -- I think this program does not work except under GopherJS: https://gopherjs.github.io/playground/#/UxD9vDjbf0

slimsag added 3 commits Dec 1, 2019
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Dec 1, 2019

Codecov Report

Merging #251 into master will increase coverage by 0.12%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #251      +/-   ##
==========================================
+ Coverage   59.31%   59.43%   +0.12%     
==========================================
  Files           4        4              
  Lines         671      673       +2     
==========================================
+ Hits          398      400       +2     
  Misses        215      215              
  Partials       58       58
Impacted Files Coverage Δ
dom.go 57.42% <100%> (ø) ⬆️
dom_native.go 52.94% <100%> (+6.27%) ⬆️

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 acd5697...7ee19ae. Read the comment docs.

// 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()

This comment has been minimized.

Copy link
@dmitshur

dmitshur Dec 1, 2019

Member

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.

This comment has been minimized.

Copy link
@slimsag

slimsag Dec 1, 2019

Author Member

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.

@slimsag slimsag merged commit 14367e2 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/nodename branch Dec 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.