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

[chrome] Add DevTools Recorder #61891

Closed
wants to merge 3 commits into from

Conversation

nickmccurdy
Copy link
Contributor

Please fill in this template.

If changing an existing definition:

@typescript-bot
Copy link
Contributor

typescript-bot commented Aug 23, 2022

@nickmccurdy Thank you for submitting this PR!

This is a live comment which I will keep updated.

1 package in this PR

Code Reviews

Because you edited one package and updated the tests (👏), I can help you merge this PR once someone else signs off on it.

You can test the changes of this PR in the Playground.

Status

  • ✅ No merge conflicts
  • ❌ Continuous integration tests have failed
  • 🕐 Most recent commit is approved by type definition owners or DT maintainers

Once every item on this list is checked, I'll ask you for permission to merge and publish the changes.

Inactive

This PR has been inactive for 31 days — it is considered abandoned, and therefore closed!


Diagnostic Information: What the bot saw about this PR
{
  "type": "info",
  "now": "-",
  "pr_number": 61891,
  "author": "nickmccurdy",
  "headCommitOid": "2a1797e4bbab0b709be586cf5223fe36c5959d27",
  "mergeBaseOid": "c696c6a733aeddf1c3211319ad3711d14e166311",
  "lastPushDate": "2022-08-23T16:26:41.000Z",
  "lastActivityDate": "2023-02-08T07:59:45.000Z",
  "hasMergeConflict": false,
  "isFirstContribution": false,
  "tooManyFiles": false,
  "hugeChange": false,
  "popularityLevel": "Popular",
  "pkgInfo": [
    {
      "name": "chrome",
      "kind": "edit",
      "files": [
        {
          "path": "types/chrome/index.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/chrome/package.json",
          "kind": "package-meta-ok"
        },
        {
          "path": "types/chrome/test/index.ts",
          "kind": "test"
        },
        {
          "path": "types/chrome/tsconfig.json",
          "kind": "package-meta-ok"
        }
      ],
      "owners": [
        "matthewkimber",
        "otiai10",
        "sreimer15",
        "MatCarlson",
        "ekinsol",
        "tregagnon",
        "echoabstract",
        "spasma",
        "bdbai",
        "pokutuna",
        "JasonXian",
        "usertim",
        "idan315",
        "nicolas377",
        "idosal"
      ],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Popular"
    }
  ],
  "reviews": [
    {
      "type": "changereq",
      "reviewer": "fregante",
      "date": "2023-02-08T07:59:45.000Z"
    }
  ],
  "mainBotCommentID": 1224099420,
  "ciResult": "fail",
  "ciUrl": "https://github.com/DefinitelyTyped/DefinitelyTyped/commit/2a1797e4bbab0b709be586cf5223fe36c5959d27/checks?check_suite_id=7944575178"
}

@typescript-bot typescript-bot added Popular package This PR affects a popular package (as counted by NPM download counts). Check Config Changes a module config files labels Aug 23, 2022
@typescript-bot typescript-bot added this to Waiting for Code Reviews in New Pull Request Status Board Aug 23, 2022
@typescript-bot
Copy link
Contributor

🔔 @matthewkimber @otiai10 @sreimer15 @matcarlson @ekinsol @tregagnon @EchoAbstract @spasma @bdbai @pokutuna @JasonXian @userTim @idan315 @nicolas377 @idosal — please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

@typescript-bot typescript-bot added the The CI failed When GH Actions fails label Aug 23, 2022
@typescript-bot typescript-bot moved this from Waiting for Code Reviews to Needs Author Action in New Pull Request Status Board Aug 23, 2022
@typescript-bot
Copy link
Contributor

@nickmccurdy The CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

Note: builds which are failing do not end up on the list of PRs for the DT maintainers to review.

@typescript-bot typescript-bot removed The CI failed When GH Actions fails Check Config Changes a module config files labels Aug 23, 2022
@typescript-bot typescript-bot moved this from Needs Author Action to Waiting for Code Reviews in New Pull Request Status Board Aug 23, 2022
@typescript-bot typescript-bot added the The CI failed When GH Actions fails label Aug 23, 2022
@typescript-bot typescript-bot moved this from Waiting for Code Reviews to Needs Author Action in New Pull Request Status Board Aug 23, 2022
@typescript-bot
Copy link
Contributor

@nickmccurdy The CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

Note: builds which are failing do not end up on the list of PRs for the DT maintainers to review.

@typescript-bot typescript-bot removed the The CI failed When GH Actions fails label Aug 23, 2022
@typescript-bot typescript-bot moved this from Needs Author Action to Waiting for Code Reviews in New Pull Request Status Board Aug 23, 2022
@typescript-bot typescript-bot added the The CI failed When GH Actions fails label Aug 23, 2022
@typescript-bot typescript-bot moved this from Waiting for Code Reviews to Needs Author Action in New Pull Request Status Board Aug 23, 2022
@typescript-bot
Copy link
Contributor

@nickmccurdy The CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

Note: builds which are failing do not end up on the list of PRs for the DT maintainers to review.

@nickmccurdy
Copy link
Contributor Author

nickmccurdy commented Aug 23, 2022

"target": "es6" doesn't seem to be working but I'm not sure why.

Error: 2> ../chrome/node_modules/@puppeteer/replay/lib/main.d.ts(442,5): error TS18028: Private identifiers are only available when targeting ECMAScript 2015 and higher.
Error: 2> ../chrome/node_modules/@puppeteer/replay/lib/main.d.ts(469,5): error TS18028: Private identifiers are only available when targeting ECMAScript 2015 and higher.
Error: 2> ../chrome/node_modules/@puppeteer/replay/lib/main.d.ts(499,5): error TS18028: Private identifiers are only available when targeting ECMAScript 2015 and higher.

@nickmccurdy
Copy link
Contributor Author

I'd appreciate help with the build errors.

@nickmccurdy
Copy link
Contributor Author

Taking another look, and the errors seem to be unrelated. Anything we can do to unblock this please?

@JasonXian
Copy link
Contributor

Hey @nickmccurdy, I played around with different tsconfig options but couldn't seem to solve the issue either. I agree that updating the target to es6 should fix the puppeteer errors but it seems to just generates other errors as well.

For now, instead of importing Step and UserFlow for the argument types, it might be simpler to use any for now. I'd assume any downstream use cases would be passing in a properly typed argument from the types in the original library anyways, meaning there would still be some safety.

This isn't an ideal solution, but I don't think this build system issue is worth being blocked on in the meanwhile.

{
"private": true,
"dependencies": {
"@puppeteer/replay": "^1.0.0"
Copy link
Contributor

@fregante fregante Feb 8, 2023

Choose a reason for hiding this comment

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

I'm not a fan of adding 1MB-plus dependency to a type package. I think this should be dropped. I think that if someone needs those types they can install it independently, even if this d.ts explicitly mentions import type {Step, UserFlow} from '@puppeteer/replay'

If that's not acceptable, how about vendoring replay’s d.ts instead? https://unpkg.com/browse/@puppeteer/replay@2.9.0/lib/main.d.ts

@typescript-bot typescript-bot added the Revision needed This PR needs code changes before it can be merged. label Feb 8, 2023
@typescript-bot
Copy link
Contributor

@nickmccurdy One or more reviewers has requested changes. Please address their comments. I'll be back once they sign off or you've pushed new commits. Thank you!

@typescript-bot typescript-bot added the Abandoned This PR had no activity for a long time, and is considered abandoned label Mar 3, 2023
@typescript-bot
Copy link
Contributor

@nickmccurdy I haven't seen any activity on this PR in more than three weeks, and it still has problems that prevent it from being merged. The PR will be closed on Mar 10th (in a week) if the issues aren't addressed.

@typescript-bot typescript-bot removed this from Needs Author Action in New Pull Request Status Board Mar 11, 2023
@typescript-bot
Copy link
Contributor

@nickmccurdy To keep things tidy, we have to close PRs that aren't mergeable and don't have activity in the last month. No worries, though — please open a new PR if you'd like to continue with this change. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Abandoned This PR had no activity for a long time, and is considered abandoned Popular package This PR affects a popular package (as counted by NPM download counts). Revision needed This PR needs code changes before it can be merged. The CI failed When GH Actions fails
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants