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 slimsag commented Dec 1, 2019

@slimsag
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
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

@codecov-io
Copy link

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

@slimsag slimsag merged commit 14367e2 into master Dec 1, 2019
@slimsag slimsag deleted the sg/nodename branch December 1, 2019 07:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants