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

Record and playback - Types #4230

Closed
HarshaNalluru opened this issue Jul 9, 2019 · 3 comments
Closed

Record and playback - Types #4230

HarshaNalluru opened this issue Jul 9, 2019 · 3 comments
Assignees
Labels
Client This issue points to a problem in the data-plane of the library. test-utils-recorder Label for the issues related to the common recorder
Milestone

Comments

@HarshaNalluru
Copy link
Member

Provide proper types for the recorder module -
image
image
image

@HarshaNalluru HarshaNalluru added the Client This issue points to a problem in the data-plane of the library. label Jul 9, 2019
@triage-new-issues triage-new-issues bot removed the triage label Jul 9, 2019
@sadasant sadasant self-assigned this Nov 21, 2019
@sadasant
Copy link
Contributor

Here are some of the things that I've spotted. Before going through this, please bring others to the conversation:

  1. Most of the methods are missing a decent tsdocs cleanup and review.
  2. async function blobToString(blob: Blob): Promise<string> uses fileReader.onloadend = (ev: any) => { which can be fileReader.onloadend = (ev: ProgressEvent<FileReader>) => {.
  3. String.prototype.padStart() is an anti-pattern. We shouldn't change the prototype of the default types.
  4. In utils.ts, move isBrowser to the top of the file (so that it is declared before it's used).
  5. In recorder.ts, (runtime === "node" && !isBrowser()) || should probably be moved to a local variable named onlyInNode, same with onlyInBrowser. It will help with the readability.
  6. In recorder.ts, skip should show a warning if TEST_MODE is not set.
  7. In recorder.ts, references to known, statically named properties shouldn't be in strings. uniqueTestInfo["uniqueName"][label] should probably be uniqueTestInfo.uniqueName.[label].
  8. customConsoleLog should probably be called differently. Something like improveBrowserLogs(), and probably only run if isBrowser.
  9. replaceableVariables should probably be called hideEnvironmentVariables.
  10. setReplacements should be called customizeRecordingWith.
  11. skipQueryParams should probably be called hideQueryParameters and behave similarly to hideEnvironmentVariables.
  12. setEnvironmentOnLoad needs a more expressive name.
  13. I kind of want to extract the logic into a more modular pattern: extract methods that do the basic operations and have the main methods invoke those more specific ones. It will help improve the specifics without having to think on the whole picture.

We should probably invite others to take a look and think about this, so we can have a better perspective on how to improve this.

@HarshaNalluru
Copy link
Member Author

Agree with many of the points raised.

In recorder.ts, skip should show a warning if TEST_MODE is not set.

what? why?

@HarshaNalluru HarshaNalluru removed their assignment Apr 10, 2020
@ramya-rao-a ramya-rao-a added the test-utils-recorder Label for the issues related to the common recorder label Aug 22, 2020
@ramya-rao-a ramya-rao-a added this to the Backlog milestone Aug 22, 2020
@HarshaNalluru HarshaNalluru modified the milestones: Backlog, [2022] July Jun 9, 2022
@HarshaNalluru HarshaNalluru self-assigned this Jun 9, 2022
@HarshaNalluru
Copy link
Member Author

V2 is nicer :)

@github-actions github-actions bot locked and limited conversation to collaborators Apr 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Client This issue points to a problem in the data-plane of the library. test-utils-recorder Label for the issues related to the common recorder
Projects
None yet
Development

No branches or pull requests

3 participants