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

Add a promises package with a New helper function #3176

Merged
merged 7 commits into from
Jul 10, 2023
Merged

Conversation

oleiade
Copy link
Member

@oleiade oleiade commented Jul 7, 2023

What?

This Pull Request introduces a promises package (under js) with a single New helper function facilitating the creation of a promise and obtaining its resolve and reject callback from Go code.

Why?

We noticed in the context of writing extensions and experimental modules that we would consistently rewrite the same function again and again. Thus, this specific PR aims at declaring this function in a reusable place within the k6 project.

None of this is definitive, and really is a proposal, if you have improvement ideas, shoot 🚀

Checklist

  • I have performed a self-review of my code.
  • I have added tests for my changes.
  • I have run linter locally (make ci-like-lint) and all checks pass.
  • I have run tests locally (make tests) and all tests pass.
  • I have commented on my code, particularly in hard-to-understand areas.

Related PR(s)/Issue(s)

ref #3165

@oleiade oleiade added this to the v0.46.0 milestone Jul 7, 2023
@oleiade oleiade self-assigned this Jul 7, 2023
@codecov-commenter
Copy link

codecov-commenter commented Jul 7, 2023

Codecov Report

Merging #3176 (e9f12c3) into master (a2fe0a8) will decrease coverage by 0.02%.
The diff coverage is 76.11%.

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

@@            Coverage Diff             @@
##           master    #3176      +/-   ##
==========================================
- Coverage   72.82%   72.80%   -0.02%     
==========================================
  Files         255      253       -2     
  Lines       19611    19663      +52     
==========================================
+ Hits        14281    14315      +34     
- Misses       4433     4446      +13     
- Partials      897      902       +5     
Flag Coverage Δ
ubuntu 72.80% <76.11%> (+0.03%) ⬆️
windows ?

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

Impacted Files Coverage Δ
js/bundle.go 85.90% <ø> (ø)
js/compiler/compiler.go 63.52% <ø> (ø)
js/modules/k6/k6.go 98.24% <ø> (ø)
js/summary.go 89.87% <ø> (ø)
lib/executor/base_config.go 84.21% <ø> (ø)
lib/executor/constant_arrival_rate.go 96.68% <ø> (ø)
lib/executor/ramping_arrival_rate.go 96.55% <ø> (ø)
lib/netext/httpext/response.go 100.00% <ø> (ø)
lib/netext/httpext/transport.go 97.08% <ø> (ø)
lib/runtime_options.go 100.00% <ø> (ø)
... and 5 more

... and 9 files with indirect coverage changes

@oleiade oleiade changed the title Add MakeHandledPromise helper function to modules package Add a promises package with a MakeHandledPromise helper function Jul 7, 2023
This helper function makes it convenient to create a promise in the
context of  k6 module/extension Go code.
mstoykov
mstoykov previously approved these changes Jul 7, 2023
Copy link
Contributor

@mstoykov mstoykov left a comment

Choose a reason for hiding this comment

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

LGTM in general.

I have some worries about the documentation. I also find the name ... strange. And I do know big parts of this come from my original code, years ago.

But promises.NewHandledPromise(vu) doesn't really tell you what "handled" means.

I don't have a particular proposition on that part though. My only thought is promises.NewEasyPromise as I guess that is the main part people care about - it is easy(ier) to use.

js/promises/promises.go Outdated Show resolved Hide resolved
js/promises/promises.go Outdated Show resolved Hide resolved
Comment on lines 11 to 13
// Calling the function will create a goja promise and return its `resolve` and `reject` callbacks, wrapped
// in such a way that it will block the k6 JS runtime's event loop from exiting before they are
// called, even if the promise isn't revoled by the time the current script ends executing.
Copy link
Contributor

Choose a reason for hiding this comment

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

The current documentation:

  1. does not explain why you would use that instead of goja.NewRuntime()
  2. Explains things that arguably are better left explain in the eventloop package - and they are.

I would propose that we do actually document why you will want to use this and simple say it uses the underlying eventloop mechanism.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point 👍🏻 Will adapt it indeed 🚀

Copy link
Member Author

Choose a reason for hiding this comment

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

Here's an attempt at improving the documentation accordingly:

// MakeHandledPromise facilitates the creation of promises within k6 modules.
//
// Whenever a k6 module function or methods returns a promise, it very likely
// should be created using this function as it ensures the promise is correctly
// managed by k6's event loop.
//
// The function returns a promise, a resolve function and a reject function. The
// promise is the one that should be returned by the function, and the `resolve`
// and `reject` functions should be used to resolve or reject the promise.
//
// Usage example:
//
//	   func myAsynchronousFunction(vu modules.VU) *goja.Promise {
//		    promise, resolve, reject := CreateAsyncFunctionPromise(vu)
//		    go func() {
//		        value, err := someAsynchronousOperation()
//				   if err != nil {
//		            reject(err)
//		            return
//		        }
//
//		        resolve(value)
//		    }()
//		    return promise
//		  }
//
// In the above example, `someAsynchronousOperation` represents an operation that might take a while to finish,
// and k6 script execution shouldn't be halted during its execution. This function ensures such an operation
// is correctly managed within k6's event loop.

WDYT? Suggestions to improve?

js/promises/promises.go Outdated Show resolved Hide resolved
Co-authored-by: Mihail Stoykov <312246+mstoykov@users.noreply.github.com>
@oleiade
Copy link
Member Author

oleiade commented Jul 7, 2023

I totally agree on the naming, and I'm happy you bring it up, what would you folks think of CreatePromise or PreparePromise, RegisterPromise (to stay along the lines of `RegisterCallback), @codebien @mstoykov ? Any other ideas?

js/promises/promises.go Outdated Show resolved Hide resolved
js/promises/promises.go Outdated Show resolved Hide resolved
oleiade and others added 3 commits July 10, 2023 09:59
Co-authored-by: Ivan <2103732+codebien@users.noreply.github.com>
Co-authored-by: Ivan <2103732+codebien@users.noreply.github.com>
Copy link
Contributor

@codebien codebien left a comment

Choose a reason for hiding this comment

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

LGTM

js/promises/promises.go Outdated Show resolved Hide resolved
Co-authored-by: Ivan <2103732+codebien@users.noreply.github.com>
@oleiade oleiade changed the title Add a promises package with a MakeHandledPromise helper function Add a promises package with a New helper function Jul 10, 2023
Copy link
Contributor

@codebien codebien left a comment

Choose a reason for hiding this comment

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

@oleiade a simple test should be added, ofc you can do it in a different low-priority PR.

@oleiade oleiade merged commit d25e858 into master Jul 10, 2023
@oleiade oleiade deleted the add-promises-helper branch July 10, 2023 08:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants