This repository has been archived by the owner on Jan 30, 2025. It is now read-only.
Refactor Sobek out of screenshots and byte pointer changes #1198
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ankur22
changed the title
inancgumus
reviewed
Jan 30, 2024
There was a problem hiding this comment.
This is great! If it isn't too much work for you, could we detach the file persisting logic out of this PR and make this PR focus only on de-gojafying? I believe adding the file persisting logic should be straightforward after this. Also, we'll make the Screenshot accept an interface later. This way, we can avoid double work, and it'll also help us keep the commit history cleaner. Thanks.
|
Yep, this was done after the persister work. I believe this PR is separate from the Persister work, although it does depend on it unfortunately. I'd rather keep this as is and merge later once the persister PRs are merged into |
ankur22
force-pushed
the
refactor/use-local-persister
branch
6 times, most recently
from
f161c6a to
3cc7c51
Compare
ankur22
force-pushed
the
refactor/screenshots-goja-bytes
branch
from
f5d84de to
ba94df5
Compare
ankur22
force-pushed
the
refactor/use-local-persister
branch
2 times, most recently
from
c2f30b9 to
ab79ae7
Compare
ankur22
force-pushed
the
refactor/screenshots-goja-bytes
branch
from
ba94df5 to
ebec386
Compare
inancgumus
reviewed
Jan 31, 2024
ankur22
force-pushed
the
refactor/screenshots-goja-bytes
branch
from
ebec386 to
0a093fd
Compare
inancgumus
reviewed
Jan 31, 2024
inancgumus
previously approved these changes
Jan 31, 2024
As far as I can tell there is no reason to return a pointer to the slice of bytes, since the slice it self is referenced with a pointer.
ankur22
force-pushed
the
refactor/screenshots-goja-bytes
branch
from
11219b4 to
f8799f2
Compare
The remote parser no longer works with goja so the result from evaluate will not return a goja value. Now just type assert without goja.
ankur22
force-pushed
the
refactor/screenshots-goja-bytes
branch
from
f8799f2 to
0cddf7c
Compare
To fix the linter issue
inancgumus
reviewed
Jan 31, 2024
inancgumus
approved these changes
Jan 31, 2024
inancgumus
changed the title
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
This refactors the parsing of the goja options for
frame.screenshotandpage.screenshotto the mapping layer, as well as moving goja out of screenshotter. This means that the screenshot business logic is free of goja.It also changes the
screenshotterto return a[]byteinstead of a pointer to it (*[]byte).Why?
The options should be parsed out of the
goja.Valuerepresentation in the mapping layer to avoid goja escaping into the business logic which makes the business logic more complex and this will also help if we convert this method into async.There's no reason to return a
*[]byteas far as i can tell.Checklist
Related PR(s)/Issue(s)
Updates: grafana/k6#4219