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

Stop capture #56

Merged
merged 3 commits into from
Jun 14, 2021
Merged

Stop capture #56

merged 3 commits into from
Jun 14, 2021

Conversation

RhinoW
Copy link
Contributor

@RhinoW RhinoW commented Mar 22, 2021

Checklist

Description

In my use case (and Issue #32) I don't know exactly how long the capture should be before hand.
This PR allows page to use window.stopCapture() to control when capture ends.

@tungs
Copy link
Owner

tungs commented Mar 29, 2021

Thanks for filing this. I like the idea of being able to stop capturing from the captured web page. There seem to be some minor issues (like typos), but my biggest concern is polluting the global namespace, so I'd prefer to use something more specific and less likely to conflict than window.stopCapture. I'll think about it this week.

@tungs tungs self-requested a review March 29, 2021 04:53
@tungs tungs added the enhancement New feature or request label Mar 29, 2021
@tungs tungs linked an issue Mar 29, 2021 that may be closed by this pull request
Copy link
Owner

@tungs tungs left a comment

Choose a reason for hiding this comment

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

I'm still thinking about what I'd prefer the name to be instead of window.stopCapture(), but here are the other changes I have in mind. I'll do another review when I decide on a different name.

index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
@RhinoW
Copy link
Contributor Author

RhinoW commented Apr 22, 2021

This may be a better way to handle - #24 - a captureWhileSelectorExists option.

@RhinoW
Copy link
Contributor Author

RhinoW commented Apr 22, 2021

Decision between

  • Add something like window.startTimesnap() window.stopTimesnap() methods - easy but pollutes global namespace
  • Ashboys captureWhileSelectorExists idea - doesn't pollute but slightly more faff from rendered page (have to create/delete some element rather than calling direct).

@tungs
Copy link
Owner

tungs commented Apr 23, 2021

After mulling over the idea of polluting the global namespace with arbitrary names, I think a somewhat cleaner implementation is to allow the user to specify the name from the command line or config. Compared to the alternatives, this still allows for the stop functionality to be called from a command line invocation, while polluting with a user-chosen name.

Copy link
Owner

@tungs tungs left a comment

Choose a reason for hiding this comment

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

Instead of using the set name stopCapture, I think it's cleaner to allow the user to specify the name of the stop function via config.stopFunctionName from node or --stop-function-name from the command line.

The command line and node API sections of README.md need this addition, as does cli.js, though I can make these changes.

Let me know what you think.

index.js Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
@tungs tungs changed the base branch from master to manual-stop-capture June 14, 2021 00:47
@tungs tungs merged commit 35b22ad into tungs:manual-stop-capture Jun 14, 2021
tungs added a commit that referenced this pull request Jun 14, 2021
* Stop capture (#56)

* adds a `config.stopFunctionName` and `--stop-function-name` which allows the user to specify a function that can be called to stop capture

* updated cli.js and README.md for stopFunctionName

Co-authored-by: RhinoW <[email protected]>
Co-authored-by: user <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can I use timesnap without the duration parameter?
2 participants