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 Goja from Execution Context Eval and from related parts #1189

Merged
merged 12 commits into from
Jan 25, 2024

Conversation

inancgumus
Copy link
Member

@inancgumus inancgumus commented Jan 24, 2024

What?

All of these changes are forced by the compiler and tests while working on ExecutionContext.Eval.

  • We can now use CDP/JS values without involving Goja.
  • Removes Goja from ExecutionContext. After this refactoring, everything started to fall apart, and we needed to refactor many parts of the code together. I couldn't split the second commit into smaller commits because it would break the build.
  • Removes Goja handling from most of the common types.
  • After this change, Goja can no longer do the conversion. So we add a generic convert helper to convert any values to type-safe values. There is another one in the test browser as a helper for the tests.

Note

These additional PRs are created that will merge into this one.

Why?

Checklist

  • I have performed a self-review of my code
  • I have added tests for my changes
  • I have commented on my code, particularly in hard-to-understand areas (not sure about this one. I added TODOs for future PR work, like returning errors)

Related PR(s)/Issue(s)

Updates: #1182, #1170

@inancgumus inancgumus changed the title Fix/goja execctx eval Fix goja execution context eval (and remote object) Jan 24, 2024
@inancgumus inancgumus marked this pull request as ready for review January 24, 2024 12:37
@inancgumus inancgumus self-assigned this Jan 24, 2024
@inancgumus inancgumus changed the title Fix goja execution context eval (and remote object) Remove Goja from Execution Context Eval and from related parts Jan 24, 2024
Copy link
Collaborator

@ankur22 ankur22 left a comment

Choose a reason for hiding this comment

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

This feels like a pretty big step forward into moving away from working with goja within the core business logic 👏

I've left comments for you to review, but I need to review it some more.

browser/mapping.go Outdated Show resolved Hide resolved
common/element_handle.go Show resolved Hide resolved
common/element_handle.go Outdated Show resolved Hide resolved
common/element_handle.go Show resolved Hide resolved
common/element_handle.go Show resolved Hide resolved
common/execution_context.go Show resolved Hide resolved
common/execution_context.go Show resolved Hide resolved
common/frame.go Show resolved Hide resolved
common/frame.go Show resolved Hide resolved
common/helpers.go Show resolved Hide resolved
Copy link
Collaborator

@ankur22 ankur22 left a comment

Choose a reason for hiding this comment

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

Great work. The code is easier to read without all that goja back-and-forth type transformation.

common/js_handle.go Show resolved Hide resolved
common/page.go Show resolved Hide resolved
common/page.go Show resolved Hide resolved
common/screenshotter.go Outdated Show resolved Hide resolved
tests/browser_context_test.go Outdated Show resolved Hide resolved
tests/element_handle_test.go Show resolved Hide resolved
tests/mouse_test.go Outdated Show resolved Hide resolved
tests/page_test.go Outdated Show resolved Hide resolved
inancgumus added a commit that referenced this pull request Jan 25, 2024
inancgumus added a commit that referenced this pull request Jan 25, 2024
inancgumus and others added 7 commits January 25, 2024 14:53
Co-authored-by: Ankur Agarwal <ankur.agarwal@grafana.com>
Co-authored-by: Ankur Agarwal <ankur.agarwal@grafana.com>
Co-authored-by: Ankur Agarwal <ankur.agarwal@grafana.com>
This commit is intentionally failing
@inancgumus inancgumus added refactor async supports async (promises) tests dx developer experience labels Jan 25, 2024
Copy link
Collaborator

@ankur22 ankur22 left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@inancgumus inancgumus merged commit 94567d9 into main-next Jan 25, 2024
14 checks passed
@inancgumus inancgumus deleted the fix/goja-execctx-eval branch January 25, 2024 14:52
inancgumus added a commit that referenced this pull request Jan 25, 2024
inancgumus added a commit that referenced this pull request Jan 25, 2024
inancgumus added a commit that referenced this pull request Jan 29, 2024
inancgumus added a commit that referenced this pull request Jan 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
async supports async (promises) dx developer experience refactor tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants