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
@inancgumus inancgumus force-pushed the fix/goja-execctx-eval branch from 00b7f9f to b34470e Compare January 25, 2024 11:55
@inancgumus inancgumus force-pushed the fix/goja-execctx-eval branch from b34470e to 3ed072b Compare January 25, 2024 11:57
@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
@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 subscribe to this conversation on GitHub. Already have an account? Sign in.

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