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

Remove the api package to use concrete common types #898

Closed
9 tasks done
ankur22 opened this issue May 22, 2023 · 0 comments · Fixed by #1057
Closed
9 tasks done

Remove the api package to use concrete common types #898

ankur22 opened this issue May 22, 2023 · 0 comments · Fixed by #1057
Assignees
Labels
productivity Issues and PRs that improve our productivity refactor

Comments

@ankur22
Copy link
Collaborator

ankur22 commented May 22, 2023

Feature Description

In Go, generally, interfaces are small and focused, as well as defined where they're being used. This also helps in testing, where we can define smaller focused interfaces which only mock methods that are used by the test, rather than a large interface where everything needs to be mocked even if majority of the methods aren't being used by the test.

The api package was useful when we required the concrete types to comply with the interface since we were returning the interfaces to the goja layer directly. E.g.:

// Ensure Browser implements the EventEmitter and Browser interfaces.
var (
	_ EventEmitter = &Browser{}
	_ api.Browser  = &Browser{}
)

Now that we're working with the mapping layer which explicitly maps the implementation to a goja type, this api compliance requirement is no longer required.

When working on recent investigative issues, we've found that this interface api compliance has become a hinderance.

This issue is too explore whether we can remove the api package and what the consequences of that would be. Something to consider are:

POC

https://github.com/grafana/xk6-browser/commits/refactor/api-package-poc


Primary tasks

  1. productivity refactor tests
    inancgumus
  2. productivity refactor
    inancgumus
  3. productivity refactor
    inancgumus

Already existing or connected issues / PRs (optional)

Related issues:

@ankur22 ankur22 added feature A new feature refactor labels May 22, 2023
@ankur22 ankur22 added this to the v0.10.0 milestone May 22, 2023
@ankur22 ankur22 mentioned this issue Aug 25, 2023
3 tasks
@inancgumus inancgumus added the next Might be eligible for the next planning (not guaranteed!) label Sep 28, 2023
@inancgumus inancgumus mentioned this issue Sep 29, 2023
3 tasks
@inancgumus inancgumus changed the title Evaluate the possibility of removing the api package Remove the api package to use concrete common types Oct 2, 2023
@inancgumus inancgumus removed this from the v0.10.0 milestone Oct 6, 2023
@inancgumus inancgumus self-assigned this Oct 6, 2023
@inancgumus inancgumus removed the next Might be eligible for the next planning (not guaranteed!) label Oct 6, 2023
@inancgumus inancgumus added dx developer experience and removed feature A new feature labels Oct 6, 2023
@inancgumus inancgumus added productivity Issues and PRs that improve our productivity and removed dx developer experience labels Oct 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
productivity Issues and PRs that improve our productivity refactor
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants