Skip to content

Conversation

@eps1lon
Copy link
Collaborator

@eps1lon eps1lon commented Aug 12, 2020

Copies current typings into v17-rc at which we'll target upcoming breaking changes.

Discussion about breaking changes should happen in #46691. This PR is only for discussion how we release @types/react@17.

If we move the current typings into v16 and let react/index.d.ts point to v17 typings then the installed types from @types/react would be 17 while react would point to 16. So we need to put 17 typings in a separate file/folder. The question is what approach the types publisher understands:

  • default typings are older then typings in a separate path

Alternatively we use the same approach as experimental.d.ts and add v17-rc.d.ts but it's unclear if dtslint would understand that this file would run with a different typescript version (planned for v17). This is not planned for React 17 but we want to do it for 18 and with this PR we can check if this approach is viable.

@typescript-bot typescript-bot added Critical package Author is Owner The author of this PR is a listed owner of the package. Check Config Changes a module config files labels Aug 12, 2020
@typescript-bot
Copy link
Contributor

typescript-bot commented Aug 12, 2020

@eps1lon Thank you for submitting this PR!

This is a live comment which I will keep updated.

1 package in this PR

Code Reviews

This PR can be merged once it's reviewed.

Status

  • ✅ No merge conflicts
  • ✅ Continuous integration tests have passed
  • ❌ 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 18 days.


Diagnostic Information: What the bot saw about this PR
{
  "type": "info",
  "now": "-",
  "pr_number": 46690,
  "author": "eps1lon",
  "owners": [
    "johnnyreilly",
    "bbenezech",
    "pzavolinsky",
    "digiguru",
    "ericanderson",
    "DovydasNavickas",
    "theruther4d",
    "guilhermehubner",
    "ferdaber",
    "jrakotoharisoa",
    "pascaloliv",
    "hotell",
    "franklixuefei",
    "Jessidhia",
    "saranshkataria",
    "lukyth",
    "eps1lon",
    "zieka",
    "dancerphil",
    "dimitropoulos",
    "disjukr",
    "vhfmag",
    "hellatan"
  ],
  "dangerLevel": "ScopedAndConfiguration",
  "headCommitAbbrOid": "c3063c4",
  "headCommitOid": "c3063c497aa4020b08bcdff8606d1f242c76544d",
  "mergeIsRequested": false,
  "stalenessInDays": 18,
  "lastPushDate": "2020-08-12T13:05:20.000Z",
  "lastCommentDate": "2020-10-02T07:31:10.000Z",
  "maintainerBlessed": true,
  "reviewLink": "https://github.com/DefinitelyTyped/DefinitelyTyped/pull/46690/files",
  "hasMergeConflict": false,
  "authorIsOwner": true,
  "isFirstContribution": false,
  "popularityLevel": "Critical",
  "newPackages": [],
  "packages": [
    "react"
  ],
  "files": [
    {
      "path": "types/react/OTHER_FILES.txt",
      "kind": "package-meta-ok",
      "package": "react"
    },
    {
      "path": "types/react/v17/OTHER_FILES.txt",
      "kind": "package-meta-ok",
      "package": "react"
    },
    {
      "path": "types/react/v17/experimental.d.ts",
      "kind": "definition",
      "package": "react"
    },
    {
      "path": "types/react/v17/global.d.ts",
      "kind": "definition",
      "package": "react"
    },
    {
      "path": "types/react/v17/index.d.ts",
      "kind": "definition",
      "package": "react"
    },
    {
      "path": "types/react/v17/package.json",
      "kind": "package-meta-ok",
      "package": "react"
    },
    {
      "path": "types/react/v17/test/cssProperties.tsx",
      "kind": "test",
      "package": "react"
    },
    {
      "path": "types/react/v17/test/elementAttributes.tsx",
      "kind": "test",
      "package": "react"
    },
    {
      "path": "types/react/v17/test/experimental.tsx",
      "kind": "test",
      "package": "react"
    },
    {
      "path": "types/react/v17/test/hooks.tsx",
      "kind": "test",
      "package": "react"
    },
    {
      "path": "types/react/v17/test/index.ts",
      "kind": "test",
      "package": "react"
    },
    {
      "path": "types/react/v17/test/managedAttributes.tsx",
      "kind": "test",
      "package": "react"
    },
    {
      "path": "types/react/v17/test/tsx.tsx",
      "kind": "test",
      "package": "react"
    },
    {
      "path": "types/react/v17/tsconfig.json",
      "kind": "package-meta",
      "package": "react",
      "suspect": "not the required form"
    },
    {
      "path": "types/react/v17/tslint.json",
      "kind": "package-meta",
      "package": "react",
      "suspect": "not the required form"
    }
  ],
  "hasDismissedReview": false,
  "ciResult": "pass",
  "reviewersWithStaleReviews": [
    {
      "reviewedAbbrOid": "0547f68",
      "reviewer": "ferdaber",
      "date": "2020-08-12T12:59:08Z"
    }
  ],
  "approvalFlags": 0,
  "isChangesRequested": false
}

@typescript-bot
Copy link
Contributor

🔔 @johnnyreilly @bbenezech @pzavolinsky @digiguru @ericanderson @DovydasNavickas @theruther4d @guilhermehubner @ferdaber @jrakotoharisoa @pascaloliv @Hotell @franklixuefei @Jessidhia @saranshkataria @lukyth @zieka @dancerphil @dimitropoulos @disjukr @vhfmag @hellatan — 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 12, 2020
@typescript-bot
Copy link
Contributor

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

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

@typescript-bot typescript-bot added Owner Approved A listed owner of this package signed off on the pull request. and removed Owner Approved A listed owner of this package signed off on the pull request. labels Aug 12, 2020
@typescript-bot
Copy link
Contributor

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

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

Seems to not be understood by npm run test
@eps1lon eps1lon force-pushed the feat/react/17-infra branch from dc76770 to c3063c4 Compare August 12, 2020 13:05
@typescript-bot typescript-bot removed the The CI failed When GH Actions fails label Aug 12, 2020
@typescript-bot
Copy link
Contributor

@ferdaber Thank you for reviewing this PR! The author has pushed new commits since your last review. Could you take another look and submit a fresh review?

@typescript-bot
Copy link
Contributor

👋 Hi there! I’ve run some quick measurements against master and your PR. These metrics should help the humans reviewing this PR gauge whether it might negatively affect compile times or editor responsiveness for users who install these typings.

Let’s review the numbers, shall we?

react/v*

Comparison details for react/* 📊
master #46690 diff
Batch compilation
Memory usage (MiB) 130.7 129.8 -0.7%
Type count 35619 35619 0%
Assignability cache size 8532 8532 0%
Language service
Samples taken 2851 2851 0%
Identifiers in tests 2851 2851 0%
getCompletionsAtPosition
    Mean duration (ms) 229.1 245.6 +7.2%
    Mean CV 9.0% 8.7%
    Worst duration (ms) 884.2 887.7 +0.4%
    Worst identifier x memoized4Ref
getQuickInfoAtPosition
    Mean duration (ms) 225.5 242.6 +7.6%
    Mean CV 9.1% 9.2% +0.9%
    Worst duration (ms) 692.4 752.6 +8.7%
    Worst identifier x props

It looks like nothing changed too much. I won’t post performance data again unless it gets worse.

react/v*

master #46690 diff
Batch compilation
Memory usage (MiB) 118.4 130.8 +10.4%
Type count 35619 35619 0%
Assignability cache size 8532 8532 0%
Language service
Samples taken 2851 2851 0%
Identifiers in tests 2851 2851 0%
getCompletionsAtPosition
    Mean duration (ms) 254.6 226.1 -11.2%
    Mean CV 9.8% 8.6%
    Worst duration (ms) 1088.1 882.9 -18.9%
    Worst identifier memoized4Ref props
getQuickInfoAtPosition
    Mean duration (ms) 250.4 222.8 -11.0%
    Mean CV 10.0% 9.1% -9.3%
    Worst duration (ms) 924.6 692.5 -25.1% 🌟
    Worst identifier ref ref

Wow, it looks like all the big movers moved in the right direction! Way to go! 🌟 I won’t post performance data again unless it gets worse.

@typescript-bot typescript-bot added the Perf: Better typescript-bot determined that this PR improves compilation performance. label Aug 12, 2020
@typescript-bot typescript-bot added Unmerged The author did not merge the PR when it was ready. and removed Unmerged The author did not merge the PR when it was ready. labels Aug 22, 2020
@eps1lon
Copy link
Collaborator Author

eps1lon commented Aug 28, 2020

@DanielRosenwasser Is this approach problematic for the infra in this repo? We started some conversation but it fizzled out (https://twitter.com/drosenwasser/status/1293702990105161729).

@typescript-bot typescript-bot added the Abandoned This PR had no activity for a long time, and is considered abandoned label Sep 29, 2020
@eps1lon
Copy link
Collaborator Author

eps1lon commented Oct 2, 2020

Not abandoned

@typescript-bot typescript-bot removed the Abandoned This PR had no activity for a long time, and is considered abandoned label Oct 2, 2020
@eps1lon
Copy link
Collaborator Author

eps1lon commented Oct 20, 2020

Superseded by #48971

@eps1lon eps1lon closed this Oct 20, 2020
@eps1lon eps1lon deleted the feat/react/17-infra branch October 20, 2020 23:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Author is Owner The author of this PR is a listed owner of the package. Check Config Changes a module config files Critical package Perf: Better typescript-bot determined that this PR improves compilation performance.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants