Refactor `k6/http` and `k6/html` to the new JS `Module` API #2226

na-- · 2021-11-08T07:35:45Z

I thought I needed to refactor both k6/html and k6/http together, since the latter depends on the former, but turns out that wasn't quite the case and the panic was caused by something else... 😅 Still, it doesn't hurt and the k6/html changes are relatively minor...

I like how this looks like for now, and I'll probably make another PR after this to move some things out of the js.Runner.newVu() and lib.State and into the k6/http module, for example:

TLSConfig, Transport and CookieJar probably should go in the Client and be initialized in a constructor, which will use the global lib.Options as the default options for its initialization
BPool can either be in the Client or ModuleInstance, depends
RPSLimit should probably be in the RootModule

As a prerequisite to these though, other built-in modules (and xk6 extensions) should be able to essentially do the equivalent of require('k6/http') internally, i.e. get the ModuleInstance for their VU. And to do that without Go import loops, we probably have to move some of the logic from js/InitContext to js/common.InitEnvironment

CLAassistant · 2021-11-16T14:24:46Z

All committers have signed the CLA.

na-- · 2021-11-16T14:40:50Z

I'll fix the linter errors and squash the commits later, but the code should be reasonably ready for review now.

js/modules/k6/http/http.go

js/modules/k6/http/tls_test.go

codebien

LGTM just some small suggestions, not too enthusiastic about the ModuleInstance <-> Client cycle dependency but I think we can't do better for now.

js/modules/k6/html/html.go

js/modules/k6/http/http.go

oleiade

LGTM 👍 A couple of nitpicks that you should feel free to ignore.

A general remark about a pattern I've observed in other places in the code: I often see private and public functions/methods/types appearing in a seamingly unordered fashion. I was always told, and exposed to the idea that especially in Go (as naming has an impact on the privateness/publicness of the code), it's important to have types/functions/methods appear in order of importance: public first, then private. Most essential/important first, less important/implementation details last. In my experience, being intentional about it makes a codebase much easier to understand, navigate and maintain.

What do you folks think?

js/modules/k6/http/request.go

mstoykov

LGTM in general, I left some comments I do think we should discuss what we want to do with Exports more in general soon so that we can align all internal modules within this release

js/modules/k6/http/request_test.go

js/modules/k6/http/request.go

lib/state.go

samples/http_get.js

js/modules/k6/http/http.go

mstoykov · 2021-11-24T17:02:42Z

js/modules/k6/http/http.go

+func (mi *ModuleInstance) Exports() modules.Exports {
+	return modules.Exports{
+		Default: mi.exports,
+		// TODO: add new HTTP APIs like Client, Request, etc. as named exports?


see #2258 as it was more or less written for this module 🤣

I started by reading that issue, but I am still not sure exactly what you mean, sorry 😞 Do you mean I should just return everything as named exports here and rely on this?

k6/js/initcontext.go

Lines 180 to 182 in 5c14dae

if exp.Default == nil {

return exp.Named

}

mstoykov · 2021-11-24T17:10:49Z

js/modules/k6/http/http.go

+	mustAddProp := func(name, val string) {
+		err := mi.exports.DefineDataProperty(
+			name, rt.ToValue(val), goja.FLAG_FALSE, goja.FLAG_FALSE, goja.FLAG_TRUE,
+		)
+		if err != nil {
+			common.Throw(rt, err)
+		}


I feel like this is the one use case for using goja.Object as the exports - being able to make things not act just as a simple map, as in this case not being able to overwrite this constants.

The question is should you not be able to overwrite this but be able to overwrite the functions ?
Should we return something different that is more robust in handling such cases

See #2241 (comment) (And above comments) for discussion on why I prefer map[string]interface{} for most cases

Hmm 🤔 I can understand the map[string]interface{} preference, but goja will just convert that to an Object anyway. And I like being able to have constants be constant 😅

Do you know hot this should work with proper ES modules? If you have export const something, does that actually make it constant in the code that imports it?

If you have export const something, does that actually make it constant in the code that imports it?

yes

codebien

Thanks for the effort in doing it, I mostly omitted to comment the parts related to the exports because I would wait for updates from @mstoykov experiments before providing any input there.

The rest is mostly asking for clarifications about TODOs.

.golangci.yml

js/modules/k6/http/file.go

js/modules/k6/http/http.go

js/modules/k6/http/cookiejar.go

js/modules/k6/http/http.go

lib/state.go

lib/netext/httpext/request_test.go

js/modules/k6/http/http_test.go

mstoykov

LGTM apart from the tests changes. I would expect that there will be some trouble with the tests either way so 🤷

codebien

Just one missing change, the rest LGTM

js/modules/k6/http/http.go

codebien

LGTM (ofc with green CI), thanks for doing it 🙏

oleiade

Just went through the PR again. Realized I forgot to approve it in the first place. Sorry for that 🙏🏻 Great work 🤝 🎉

na-- added this to the v0.36.0 milestone Nov 8, 2021

na-- requested a review from mstoykov November 8, 2021 07:35

na-- force-pushed the http-js-refactor branch from 1c8ddda to d4ea55c Compare November 15, 2021 15:43

na-- changed the title ~~[WIP] Refactor k6/http to ModuleV2~~ [WIP] Refactor k6/http and k6/html to the new JS Module API Nov 16, 2021

na-- changed the title ~~[WIP] Refactor k6/http and k6/html to the new JS Module API~~ Refactor k6/http and k6/html to the new JS Module API Nov 16, 2021

na-- marked this pull request as ready for review November 16, 2021 14:39

na-- requested review from oleiade and codebien November 16, 2021 14:40

mstoykov reviewed Nov 16, 2021

View reviewed changes

js/modules/k6/http/http.go Show resolved Hide resolved

mstoykov reviewed Nov 16, 2021

View reviewed changes

js/modules/k6/http/tls_test.go Outdated Show resolved Hide resolved

codebien reviewed Nov 17, 2021

View reviewed changes

js/modules/k6/html/html.go Show resolved Hide resolved

js/modules/k6/http/http.go Show resolved Hide resolved

oleiade previously approved these changes Nov 23, 2021

View reviewed changes

js/modules/k6/http/request.go Outdated Show resolved Hide resolved

js/modules/k6/http/request.go Show resolved Hide resolved

mstoykov previously approved these changes Nov 24, 2021

View reviewed changes

na-- dismissed stale reviews from mstoykov and oleiade via d87b4d6 December 3, 2021 16:59

na-- force-pushed the http-js-refactor branch from c6da5bc to d87b4d6 Compare December 3, 2021 16:59

na-- requested review from codebien, mstoykov and oleiade December 3, 2021 17:01

codebien requested changes Dec 7, 2021

View reviewed changes

oleiade mentioned this pull request Dec 13, 2021

Make netext httpext Response's Request a pointer #2271

Merged

mstoykov reviewed Dec 13, 2021

View reviewed changes

lib/netext/httpext/request_test.go Outdated Show resolved Hide resolved

mstoykov reviewed Dec 13, 2021

View reviewed changes

js/modules/k6/http/http_test.go Show resolved Hide resolved

mstoykov reviewed Dec 13, 2021

View reviewed changes

This was referenced Dec 13, 2021

Allow one pseudo-JS module to access the ModuleInstance of another module #2293

Open

Update golangci-lint and adjust settings for .golangci.yml #2294

Closed

na-- force-pushed the http-js-refactor branch 2 times, most recently from f072e4d to 18919f3 Compare December 13, 2021 14:02

na-- force-pushed the http-js-refactor branch from 18919f3 to 8a475ab Compare December 13, 2021 14:06

na-- requested review from codebien and mstoykov December 13, 2021 14:06

mstoykov previously approved these changes Dec 13, 2021

View reviewed changes

codebien requested changes Dec 13, 2021

View reviewed changes

js/modules/k6/http/http.go Outdated Show resolved Hide resolved

na-- dismissed mstoykov’s stale review via 2d45769 December 13, 2021 14:55

na-- force-pushed the http-js-refactor branch from 8a475ab to 2d45769 Compare December 13, 2021 14:55

na-- requested review from codebien and mstoykov December 13, 2021 14:56

codebien previously approved these changes Dec 13, 2021

View reviewed changes

mstoykov previously approved these changes Dec 13, 2021

View reviewed changes

oleiade previously approved these changes Dec 13, 2021

View reviewed changes

Refactor the k6/html and k6/http modules to the new JS module API

7b34a8d

na-- dismissed stale reviews from oleiade, mstoykov, and codebien via 7b34a8d December 13, 2021 15:25

na-- force-pushed the http-js-refactor branch from 2d45769 to 7b34a8d Compare December 13, 2021 15:25

mstoykov approved these changes Dec 13, 2021

View reviewed changes

codebien approved these changes Dec 13, 2021

View reviewed changes

na-- merged commit 3bcdabb into master Dec 13, 2021

na-- deleted the http-js-refactor branch December 13, 2021 15:31

This was referenced Feb 28, 2022

Revert k6/http.head to not taking body as second argument #2402

Merged

Issue with HEAD requests in 0.36.0? #2401

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor `k6/http` and `k6/html` to the new JS `Module` API #2226

Refactor `k6/http` and `k6/html` to the new JS `Module` API #2226

na-- commented Nov 8, 2021 •

edited

Loading

CLAassistant commented Nov 16, 2021 •

edited

Loading

na-- commented Nov 16, 2021

codebien left a comment

oleiade left a comment

mstoykov left a comment

mstoykov Nov 24, 2021

na-- Dec 3, 2021

mstoykov Nov 24, 2021

na-- Dec 3, 2021

mstoykov Dec 13, 2021

codebien left a comment •

edited

Loading

mstoykov left a comment

codebien left a comment

codebien left a comment

oleiade left a comment

Refactor k6/http and k6/html to the new JS Module API #2226

Refactor k6/http and k6/html to the new JS Module API #2226

Conversation

na-- commented Nov 8, 2021 • edited Loading

CLAassistant commented Nov 16, 2021 • edited Loading

na-- commented Nov 16, 2021

codebien left a comment

Choose a reason for hiding this comment

oleiade left a comment

Choose a reason for hiding this comment

mstoykov left a comment

Choose a reason for hiding this comment

mstoykov Nov 24, 2021

Choose a reason for hiding this comment

na-- Dec 3, 2021

Choose a reason for hiding this comment

mstoykov Nov 24, 2021

Choose a reason for hiding this comment

na-- Dec 3, 2021

Choose a reason for hiding this comment

mstoykov Dec 13, 2021

Choose a reason for hiding this comment

codebien left a comment • edited Loading

Choose a reason for hiding this comment

mstoykov left a comment

Choose a reason for hiding this comment

codebien left a comment

Choose a reason for hiding this comment

codebien left a comment

Choose a reason for hiding this comment

oleiade left a comment

Choose a reason for hiding this comment

Refactor `k6/http` and `k6/html` to the new JS `Module` API #2226

Refactor `k6/http` and `k6/html` to the new JS `Module` API #2226

na-- commented Nov 8, 2021 •

edited

Loading

CLAassistant commented Nov 16, 2021 •

edited

Loading

codebien left a comment •

edited

Loading