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

Refactor js modules system in js/modules package and use it #2991

Merged
merged 4 commits into from May 29, 2023

Conversation

mstoykov
Copy link
Collaborator

@mstoykov mstoykov commented Mar 29, 2023

This is mostly a stepping stone to let it be used in tests but also to make the js package smaller.

This also has a separate commit that uses it in k6/experimental/tracing where this actually matters.

@codecov-commenter
Copy link

codecov-commenter commented Mar 29, 2023

Codecov Report

Merging #2991 (ad9dad2) into master (ac96f6a) will decrease coverage by 1.69%.
The diff coverage is 27.34%.

❗ Current head ad9dad2 differs from pull request most recent head 9392a59. Consider uploading reports for the commit 9392a59 to get more accurate results

@@            Coverage Diff             @@
##           master    #2991      +/-   ##
==========================================
- Coverage   75.20%   73.52%   -1.69%     
==========================================
  Files         236      239       +3     
  Lines       17722    18244     +522     
==========================================
+ Hits        13328    13414      +86     
- Misses       3536     3959     +423     
- Partials      858      871      +13     
Flag Coverage Δ
ubuntu 73.52% <27.34%> (-1.69%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
js/initcontext.go 88.88% <ø> (+8.88%) ⬆️
js/modules/gomodule.go 77.27% <ø> (ø)
output/cloud/expv2/pbcloud/metric.pb.go 7.58% <7.58%> (ø)
output/cloud/expv2/metrics_client.go 58.00% <58.00%> (ø)
js/modules/require_impl.go 76.47% <76.47%> (ø)
js/bundle.go 85.09% <88.23%> (-0.15%) ⬇️
js/modules/resolution.go 90.32% <90.32%> (ø)
cloudapi/client.go 72.91% <100.00%> (ø)
js/modules/cjsmodule.go 78.26% <100.00%> (ø)
js/modulestest/runtime.go 100.00% <100.00%> (ø)

... and 2 files with indirect coverage changes

na--
na-- previously approved these changes Apr 3, 2023
Copy link
Member

@na-- na-- left a 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 all of the subtle details here, nor do I fully grok the desired end state yet, but this change LGTM on its own.

@mstoykov mstoykov changed the title WIP Refactor js modules system in js/modules package and use it Apr 27, 2023
@mstoykov mstoykov added this to the v0.45.0 milestone Apr 27, 2023
@mstoykov mstoykov marked this pull request as ready for review April 27, 2023 13:52
js/modules/resolution.go Show resolved Hide resolved
js/bundle.go Outdated Show resolved Hide resolved
Copy link
Contributor

@imiric imiric left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the refactor, but don't have the full context of why this is needed, besides being neater (which is not a reason to not do it).

I just left minor suggestions and questions, and mostly typo fixes.

js/initcontext.go Outdated Show resolved Hide resolved
js/modules.go Outdated Show resolved Hide resolved
js/modules/cjsmodule.go Outdated Show resolved Hide resolved
js/modules/require_impl.go Outdated Show resolved Hide resolved
js/modules/resolution.go Outdated Show resolved Hide resolved
js/modules/resolution.go Outdated Show resolved Hide resolved
js/modules/resolution.go Outdated Show resolved Hide resolved
js/modulestest/runtime.go Outdated Show resolved Hide resolved
Comment on lines -62 to +69
m := new(RootModule).NewModuleInstance(ts.VU)

rt := ts.VU.Runtime()
require.NoError(t, rt.Set("instrumentHTTP", m.Exports().Named["instrumentHTTP"]))

export := http.New().NewModuleInstance(ts.VU).Exports().Default
require.NoError(t, rt.Set("require", func(module string) *goja.Object {
require.Equal(t, "k6/http", module)
return rt.ToValue(export).ToObject(rt)
}))
err := ts.SetupModuleSystem(map[string]interface{}{
"k6/http": http.New(),
"k6/experimental/tracing": new(RootModule),
}, nil, compiler.New(ts.VU.InitEnvField.Logger))
require.NoError(t, err)

_, err = ts.VU.Runtime().RunString("var instrumentHTTP = require('k6/experimental/tracing').instrumentHTTP")
require.NoError(t, err)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, much cleaner 👍

This is mostly a stepping stone to let it be used in tests but also to
make the js package smaller
This is the main place where the module system being available
and working the same in tests as it does when k6 runs is really
important for the test itself.
@mstoykov
Copy link
Collaborator Author

mstoykov commented May 26, 2023

Sorry @olegbespalov it take me a while to rebase and squash the fixes in the correct commits as you can see in https://github.com/grafana/k6/compare/e9a1e2f4ec5f0150b7908d637d1eb24f3d4f61e7..51ee0214bbfe7d8b00664ce776ed917aa804eeca

I also forcepushed to rebase on master to fix the go-tip tests 😬

Copy link
Collaborator

@olegbespalov olegbespalov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

@mstoykov mstoykov merged commit 677bb29 into master May 29, 2023
22 checks passed
@mstoykov mstoykov deleted the moveModuleResolution branch May 29, 2023 08:25
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

5 participants