-
Notifications
You must be signed in to change notification settings - Fork 12
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
Move to the new k6 Module API #13
Conversation
see grafana/k6#2344 for more info
remote_write.go
Outdated
vu modules.VU | ||
} | ||
|
||
type root struct{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if there are any conventions around the naming of this root
type which I'm not aware of, but when I first looked at it I didn't know what root
is, so I'd consider renaming it to something more explanatory like remoteWriteModule
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no real concetion I just was lazy and didn't want to write more ... :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in 1f12b8b
func (r *RemoteWrite) XSample(value float64, timestamp int64) Sample { | ||
func (r *RemoteWrite) sample(c goja.ConstructorCall) *goja.Object { | ||
rt := r.vu.Runtime() | ||
call, _ := goja.AssertFunction(rt.ToValue(xsample)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as far as I understand the second argument indicates whether goja.AssertFunction()
succeeded, should this be checked?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only way for it to not succed is if what we provided isn't a function ... and it is ;) So this is one of those cases where the API returns an error that we know it can't have. Also the only reasonable thing to do here will be to panic, which will happen when call
is used either way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, makes sense, thanks :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TBH I didn't spend a lot of time learning the new API, but apart from what I commented on the code looks good to me
see grafana/k6#2344 for more info