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

Feature/jquery Extend jquery api for #21 #197

Merged
merged 93 commits into from Oct 26, 2017

Conversation

Projects
None yet
4 participants
@mitnuh
Copy link
Contributor

commented May 4, 2017

re: #21

About the return types - most of the current api returned a plain ol' go type but the attr() method returns a goja type. Which would you prefer I use?

@liclac

This comment has been minimized.

Copy link
Collaborator

commented May 4, 2017

Good start!

For goja.Value vs plain types; goja.Value is used to be able to return goja.Undefined() where jQuery would return undefined.

func optionVal(s *goquery.Selection) string {
val, exists := s.Attr("value")

if exists {

This comment has been minimized.

Copy link
@liclac

liclac May 4, 2017

Collaborator

imo the code is easier to read with no newline above this, same for eg.

thing, err := Fn()
if err != nil {
  //
}

Makes it immediately obvious which lines are related to each other.


_, exists := s.sel.Attr("multiple")

if exists {

This comment has been minimized.

Copy link
@liclac

liclac May 4, 2017

Collaborator

This can be shortened to if _, exists := s.sel.Attr("multiple"); exists

return s.rt.ToValue(optionVal(selected))
}

case "":

This comment has been minimized.

Copy link
@liclac

liclac May 4, 2017

Collaborator

This is kinda redundant, since it just does the same thing as the default case.

This comment has been minimized.

Copy link
@mitnuh

mitnuh May 4, 2017

Author Contributor

Ah yes, that was pretty dumb. Will fix them all and do some more tomorrow, thank you!

mitnuh added a commit to mitnuh/k6 that referenced this pull request May 5, 2017

@mitnuh

This comment has been minimized.

Copy link
Contributor Author

commented May 5, 2017

Phew for the tip about AssertFunction() in the issue thread.

I'm unsure how to handle an invalid fn argument in Each(). Wanted throw a js runtime TypeError but ended up using runtime.Interrupt.

@mitnuh
Copy link
Contributor Author

left a comment

The goquery implementation of Each and Map differ from the jquery api.

goquery passes a selection to Each() and expects a string from the Map callback. jquery passes the Element to Each and is more flexible about the return type the Map.

Will use with the goquery api for now as I'm not sure how to match the jquery api.

@liclac

This comment has been minimized.

Copy link
Collaborator

commented May 5, 2017

Try panicking with a rt.NewGoError(err) instead, that'll work right even inside a JS->Go->JS callback->Go call stack

@mitnuh

This comment has been minimized.

Copy link
Contributor Author

commented May 5, 2017

That sounds perfect, thanks again. I'm done for today, will be doing more tomorrow.

@mitnuh

This comment has been minimized.

Copy link
Contributor Author

commented May 9, 2017

There's a issue with numeric types returned by the data() function. Need the docs data-attr="101" returns int, and values "101.00" and "1E02" remain as strings but numeric values in JSON strings like data-attr='{"id":101}' are delegated to JSON.parse.

Parsing the json Go's json unmarshal returns a float64, not an issue inside javascript but the it makes the test look inconsistent and feels wrong.

@mitnuh mitnuh closed this May 9, 2017

@mitnuh mitnuh reopened this May 9, 2017

@mitnuh

This comment has been minimized.

Copy link
Contributor Author

commented May 9, 2017

sorry for the noise. I wanted to cancel a comment I was writing not close the pull request as I'd realised the answer to something while I was asking.

@liclac

This comment has been minimized.

Copy link
Collaborator

commented May 9, 2017

Hmm, that does sound a little annoying, but the problem should be purely cosmetic - JS only has a single Number type, so within JS, 1 === 1.0.

@mitnuh

This comment has been minimized.

Copy link
Contributor Author

commented May 9, 2017

Yeah, it was fine in javascript but made the go tests inconsistent. it was a mistake to return an int from convert - returning float is as valid and more consistent.

So proud to figure that out while typing but not how to close a comment without closing the PR.

@ragnarlonn

This comment has been minimized.

Copy link

commented May 9, 2017

@mitnuh Hey Tim. Just chatted with @liclac and we're impressed with your work on this so far, plus we realized the task was probably a little bigger than we initially anticipated, so we're going to increase the bounty on it. Just so you know.

@mitnuh

This comment has been minimized.

Copy link
Contributor Author

commented May 11, 2017

Is there a way to make javascript accessors to wrap go functions? Being able to use accessors for the traversal properties on the dom.Element wrapper would be really useful.

@liclac

This comment has been minimized.

Copy link
Collaborator

commented May 11, 2017

Sure - just cast them to a JS value and they'll do the right thing.

The one pitfall to look out for is that you can't do (type, error) or inject contexts in anything but top-level module functions; those are bootstrapped by common.Proxy(), and the reflection-based wrapping that entrails is far too slow to do in a hot path.

@mitnuh

This comment has been minimized.

Copy link
Contributor Author

commented May 11, 2017

@mitnuh

This comment has been minimized.

Copy link
Contributor Author

commented May 11, 2017

I'll tidy up and send some code for you to look at. Might be a while as I've got some admin that needs doing.

@liclac

This comment has been minimized.

Copy link
Collaborator

commented May 11, 2017

Oh, sorry, I misread your question - unfortunately you can't make accessors, at least not without a whole JS-side kludge :(

@mitnuh

This comment has been minimized.

Copy link
Contributor Author

commented May 11, 2017

np wasn't clear the first time - will work round it for now.

@mitnuh

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2017

Hi. Would a JS kludge like this be acceptable?

https://gist.github.com/mitnuh/b5b4bf16ec7c24d0853f5df23bcc8759

Left a note at the bottom of the gist.

@liclac

This comment has been minimized.

Copy link
Collaborator

commented May 16, 2017

Not a bad idea, go right ahead! Just make sure you compile the JS blurb into a goja.Program in an init() function, or it will be horribly slow.

If you have a moment, could you write a benchmark comparing your JS blurb vs NewObject() and Set()'ing every attribute individually vs that but requiring parentheses on childNodes(), etc? I don't think performance should be a problem here, but I'd like to be sure.

@mitnuh

This comment has been minimized.

Copy link
Contributor Author

commented May 17, 2017

Will do! I'm about to push to current version then do the benchmarks in a jquery-benchmark branch. Will try out a version with accessors, a version with Set()'ing attributes, and a version using a go object and no js kludges.

@mitnuh

This comment has been minimized.

Copy link
Contributor Author

commented May 17, 2017

Performed as you expected, ditching the JS kludge was the only version that was fast and could run the benchmarks with over 10000 iterations.

It seems to hit a resource limit (edit: I needed to increase benchtime - deleted a few lines where I rambled on about how it is probably was a memory problem)

Results moved here: https://gist.github.com/mitnuh/bd5552d9e9d3b265c9bc9ceb9844a970

@mitnuh

This comment has been minimized.

Copy link
Contributor Author

commented May 17, 2017

https://github.com/mitnuh/k6/blob/feature/jquery-benchmark/js/modules/k6/html/speed_test.go

The other versions of the benchmark are the previous half dozen commits.

@mitnuh

This comment has been minimized.

Copy link
Contributor Author

commented May 19, 2017

The current version uses a compiled kludge - the benchmarks were a bit meh so would you prefer to lose the kludge and use a go struct?

I have a problem with Namespace related properties, they're still todo because the Namespace attribute on html.Node is always empty. I'll investigate why again but if you have any ideas... My backup plan if unsuccessful is imitate NamespaceURI and isDefaultNamespace by parsing the tag & attr names.

Is there anything else that looks wrong or incomplete?

@liclac

This comment has been minimized.

Copy link
Collaborator

commented May 19, 2017

Okay so, those benchmarks are a good start, but need a bit of work - have a look at this for inspiration. Instead of running the tests manually like that, use relevant go test flags, and I'd like to see memory stats as well.

@mitnuh

This comment has been minimized.

Copy link
Contributor Author

commented May 19, 2017

Which flags are relevant? I was manually running 'go test -test.bench "BuildSpeed" -run ^$ -v'.

Will add the memory stats and redo the tests so you can compare the variants in one run.

@liclac

This comment has been minimized.

Copy link
Collaborator

commented May 19, 2017

You probably want -benchtime to up the test duration a fair bit.

@mitnuh mitnuh force-pushed the mitnuh:feature/jquery branch from d78712a to 5ff9d70 Oct 25, 2017

@liclac

liclac approved these changes Oct 26, 2017

@liclac liclac merged commit a25b94f into loadimpact:master Oct 26, 2017

1 check passed

ci/circleci Your tests passed on CircleCI!
Details
@liclac

This comment has been minimized.

Copy link
Collaborator

commented Oct 26, 2017

FINALLY! 🎉

Sorry that it took so long! The sheer size of the code made it somewhat difficult to review orz

@mitnuh

This comment has been minimized.

Copy link
Contributor Author

commented Oct 26, 2017

Hurrah! But it might have taken even longer without your advice and the code is more consistent and always improved by your reviews. So sorry about the orz. Thank you!

@robingustafsson

This comment has been minimized.

Copy link
Member

commented Jun 6, 2018

@mitnuh Sorry to contact you like this, but I can't find another way to reach you as I don't have an email adress or other contact information.

We've introduced a Contributor License Agreement for k6, and are reaching out to all contributors to ask them for their signature.

You can review and sign the CLA here: https://cla-assistant.io/loadimpact/k6

Hope this message reaches you and that there won't be any hinderances to get the CLA signed 😃

You can reach me at robin@loadimpact.com or via k6 Slack (I'm "robin" in there) if you have any questions. Thank you!

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