Skip to content
This repository was archived by the owner on Jan 30, 2025. It is now read-only.

Conversation

inancgumus
Copy link
Collaborator

@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 requested review from ankur22 and ka3de 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.

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.

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 inancgumus requested a review from ankur22 January 25, 2024 07:43