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

Type upgrades for @tonic-ui/react v2 and @tonic-ui/react-icons #69669

Closed
wants to merge 21 commits into from

Conversation

ZachLegros
Copy link
Contributor

Please fill in this template.

Select one of these and delete the others:

If adding a new definition:

  • The package does not already provide its own types, or cannot have its .d.ts files generated via --declaration
  • If this is for an npm package, match the name. If not, do not conflict with the name of an npm package.
  • Create it with dts-gen --dt, not by basing it on an existing project.
  • Represents shape of module/library correctly
  • tsconfig.json should have noImplicitAny, noImplicitThis, strictNullChecks, and strictFunctionTypes set to true.

If changing an existing definition:

@typescript-bot
Copy link
Contributor

typescript-bot commented May 23, 2024

@ZachLegros Thank you for submitting this PR!

This is a live comment which I will keep updated.

2 packages in this PR

Code Reviews

This PR adds a new definition, so it needs to be reviewed by a DT maintainer before it can be merged.

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

Status

  • ✅ No merge conflicts
  • ❌ Continuous integration tests have failed
  • 🕐 Only a DT maintainer can approve changes when there are new packages added

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


Diagnostic Information: What the bot saw about this PR
{
  "type": "info",
  "now": "-",
  "pr_number": 69669,
  "author": "ZachLegros",
  "headCommitOid": "b79b895a38119b47db0190f3f0fd6877be4ff8be",
  "mergeBaseOid": "f6d4d15522356eba4a0267142834e3abc6b603fc",
  "lastPushDate": "2024-05-23T13:17:44.000Z",
  "lastActivityDate": "2024-05-23T23:54:13.000Z",
  "hasMergeConflict": false,
  "isFirstContribution": false,
  "tooManyFiles": false,
  "hugeChange": false,
  "popularityLevel": "Well-liked by everyone",
  "pkgInfo": [
    {
      "name": "tonic-ui__react-icons",
      "kind": "add",
      "files": [
        {
          "path": "types/tonic-ui__react-icons/.eslintrc.json",
          "kind": "package-meta",
          "suspect": "edited"
        },
        {
          "path": "types/tonic-ui__react-icons/.npmignore",
          "kind": "package-meta-ok"
        },
        {
          "path": "types/tonic-ui__react-icons/index.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/tonic-ui__react-icons/package.json",
          "kind": "package-meta-ok"
        },
        {
          "path": "types/tonic-ui__react-icons/scripts/generate-definitions/.eslintrc.json",
          "kind": "package-meta",
          "suspect": "edited"
        },
        {
          "path": "types/tonic-ui__react-icons/scripts/generate-definitions/README.md",
          "kind": "markdown"
        },
        {
          "path": "types/tonic-ui__react-icons/scripts/generate-definitions/base.d.ts.template",
          "kind": "package-meta",
          "suspect": "edited"
        },
        {
          "path": "types/tonic-ui__react-icons/scripts/generate-definitions/icon.d.ts.template",
          "kind": "package-meta",
          "suspect": "edited"
        },
        {
          "path": "types/tonic-ui__react-icons/scripts/generate-definitions/index.ts",
          "kind": "test"
        },
        {
          "path": "types/tonic-ui__react-icons/scripts/generate-definitions/package.json",
          "kind": "package-meta-ok"
        },
        {
          "path": "types/tonic-ui__react-icons/scripts/generate-definitions/tmicon.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/tonic-ui__react-icons/scripts/generate-definitions/tsconfig.json",
          "kind": "package-meta",
          "suspect": "couldn't parse json"
        },
        {
          "path": "types/tonic-ui__react-icons/tonic-ui__react-icons-tests.tsx",
          "kind": "test"
        },
        {
          "path": "types/tonic-ui__react-icons/tsconfig.json",
          "kind": "package-meta",
          "suspect": "not [the expected form](https://github.com/DefinitelyTyped/DefinitelyTyped#user-content-tsconfigjson) (check: `compilerOptions.types`)"
        }
      ],
      "owners": [],
      "addedOwners": [
        "ZachLegros",
        "derekhawker"
      ],
      "deletedOwners": [],
      "popularityLevel": "Well-liked by everyone"
    },
    {
      "name": "tonic-ui__react",
      "kind": "edit",
      "files": [
        {
          "path": "types/tonic-ui__react/index.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/tonic-ui__react/package.json",
          "kind": "package-meta-ok"
        }
      ],
      "owners": [
        "derekhawker",
        "ZachLegros"
      ],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Well-liked by everyone"
    }
  ],
  "reviews": [
    {
      "type": "changereq",
      "reviewer": "jakebailey",
      "date": "2024-05-23T23:13:20.000Z"
    }
  ],
  "mainBotCommentID": 2127089876,
  "ciResult": "fail",
  "ciUrl": "https://github.com/DefinitelyTyped/DefinitelyTyped/commit/b79b895a38119b47db0190f3f0fd6877be4ff8be/checks?check_suite_id=24120719376"
}

@typescript-bot typescript-bot added New Definition This PR creates a new definition package. Edits multiple packages Check Config Changes a module config files labels May 23, 2024
@typescript-bot typescript-bot added this to Waiting for Code Reviews in New Pull Request Status Board May 23, 2024
@typescript-bot
Copy link
Contributor

🔔 @derekhawker — 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 May 23, 2024
@typescript-bot typescript-bot moved this from Waiting for Code Reviews to Needs Author Action in New Pull Request Status Board May 23, 2024
@typescript-bot
Copy link
Contributor

@ZachLegros 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 May 23, 2024
@typescript-bot typescript-bot moved this from Needs Author Action to Waiting for Code Reviews in New Pull Request Status Board May 23, 2024
@typescript-bot typescript-bot added the The CI failed When GH Actions fails label May 23, 2024
@typescript-bot
Copy link
Contributor

@ZachLegros 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 moved this from Waiting for Code Reviews to Needs Author Action in New Pull Request Status Board May 23, 2024
@typescript-bot
Copy link
Contributor

@ZachLegros 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 May 23, 2024
@typescript-bot typescript-bot moved this from Needs Author Action to Waiting for Code Reviews in New Pull Request Status Board May 23, 2024
@typescript-bot typescript-bot added the The CI failed When GH Actions fails label May 23, 2024
@typescript-bot typescript-bot moved this from Waiting for Code Reviews to Needs Author Action in New Pull Request Status Board May 23, 2024
@typescript-bot
Copy link
Contributor

@ZachLegros 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 May 23, 2024
@typescript-bot typescript-bot moved this from Needs Author Action to Waiting for Code Reviews in New Pull Request Status Board May 23, 2024
@typescript-bot typescript-bot added the The CI failed When GH Actions fails label May 23, 2024
@typescript-bot
Copy link
Contributor

@ZachLegros 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 moved this from Waiting for Code Reviews to Needs Author Action in New Pull Request Status Board May 23, 2024
@typescript-bot typescript-bot removed the The CI failed When GH Actions fails label May 23, 2024
@typescript-bot typescript-bot moved this from Needs Author Action to Waiting for Code Reviews in New Pull Request Status Board May 23, 2024
@typescript-bot
Copy link
Contributor

@ZachLegros 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 moved this from Waiting for Code Reviews to Needs Author Action in New Pull Request Status Board May 23, 2024
Comment on lines +1 to +3
declare module '@trendmicro/tmicon' {
const icons: Array<{ name: string }>;
}
Copy link
Member

Choose a reason for hiding this comment

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

The scripts can't contain dts files, they will be published. You'll need to remove this file.

Copy link
Member

@jakebailey jakebailey left a comment

Choose a reason for hiding this comment

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

@ZachLegros You work at Trend Micro, so presumably you also publish the packages in the @trendmicro and @tonic-ui scope. Can you just publish your types directly? DT is mainly for people contributing types to packages they don't own and can't add types to, but it's a little weird to publish types for packages you own on DT.

@typescript-bot
Copy link
Contributor

@ZachLegros 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 Revision needed This PR needs code changes before it can be merged. label May 23, 2024
@ZachLegros
Copy link
Contributor Author

ZachLegros commented May 23, 2024

@ZachLegros You work at Trend Micro, so presumably you also publish the packages in the @trendmicro and @tonic-ui scope. Can you just publish your types directly? DT is mainly for people contributing types to packages they don't own and can't add types to, but it's a little weird to publish types for packages you own on DT.

I do work at Trend Micro, but the @trendmicro/ and @tonic-ui/ repos are owned by other teams. Me and my coworker are writing types for @tonic-ui/ independently since TonicUI is a JS-only codebase and doesn't have plans to add TS to the library in the near future. Also, this PR aims to add typings to a new package react-icons from TonicUI, but it it is still in alpha. Should I close this PR and reopen it when the official version is published?

Thank you.

@jakebailey
Copy link
Member

I would split them, yes. DT doesn't typically allow types for prereleases.

@ZachLegros ZachLegros closed this May 24, 2024
@typescript-bot typescript-bot removed this from Needs Author Action in New Pull Request Status Board May 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Check Config Changes a module config files Edits multiple packages New Definition This PR creates a new definition package. 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

3 participants