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

[react-native] realign the RN types dependency to the right React version counterpart #61081

Merged
merged 3 commits into from Jul 11, 2022

Conversation

kelset
Copy link
Contributor

@kelset kelset commented Jul 4, 2022

  • Use a meaningful title for the pull request. Include the name of the package modified.
  • Test the change in your own code. (Compile and run.)
  • Add or edit tests to reflect the change.
  • Follow the advice from the readme.
  • Avoid common mistakes.
  • Run npm test <package to test>.
  • Provide a URL to documentation or source code which provides context for the suggested changes: (added below)

This PR does some "maintainance work" for the @types/react-native (so no new tests have been added) - basically, one core issue currently with RN types is that they all depend on latest of @types/react.

This PR attends to that in the way that the README suggest as ideal (at the FAQ question "If a library is updated to a new major version with breaking changes, how should I update its type declaration package?"), by changing the dependency in the tsconfig - before proceeding I also asked in the TS Discord in the #definitely-typed channel.

Considering that RN 0.69, the current latest/main, is the first version to support React 18, I've lowered the version of the versions 68 -> 64 to @types/react 17 (64 was the first version to support it) and of 63 to @types/react 16.

On top of this, I've also added myself to the list of owners/authors of the package so that I can keep an eye on the RN types moving forward :)

@kelset kelset requested a review from alloy as a code owner July 4, 2022 15:39
@typescript-bot
Copy link
Contributor

typescript-bot commented Jul 4, 2022

@kelset Thank you for submitting this PR!

This is a live comment which I will keep updated.

1 package in this PR

@kelset: I see that you have added yourself as an owner, are you sure you want to become an owner?

Code Reviews

This PR can be merged once it's reviewed by a DT maintainer.

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 without tests

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": 61081,
  "author": "kelset",
  "headCommitOid": "16e5f757cdec529e237c25e3931438c87ec39c51",
  "mergeBaseOid": "3a535ec0443f807beda159902a877df2fb5e6dea",
  "lastPushDate": "2022-07-04T15:25:13.000Z",
  "lastActivityDate": "2022-07-11T16:43:42.000Z",
  "hasMergeConflict": false,
  "isFirstContribution": false,
  "tooManyFiles": false,
  "hugeChange": false,
  "popularityLevel": "Popular",
  "pkgInfo": [
    {
      "name": "react-native",
      "kind": "edit",
      "files": [
        {
          "path": "types/react-native/index.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/react-native/v0.63/tsconfig.json",
          "kind": "package-meta-ok"
        },
        {
          "path": "types/react-native/v0.64/tsconfig.json",
          "kind": "package-meta-ok"
        },
        {
          "path": "types/react-native/v0.65/tsconfig.json",
          "kind": "package-meta-ok"
        },
        {
          "path": "types/react-native/v0.66/tsconfig.json",
          "kind": "package-meta-ok"
        },
        {
          "path": "types/react-native/v0.67/tsconfig.json",
          "kind": "package-meta-ok"
        },
        {
          "path": "types/react-native/v0.68/tsconfig.json",
          "kind": "package-meta-ok"
        }
      ],
      "owners": [
        "alloy",
        "huhuanming",
        "iRoachie",
        "timwangdev",
        "kamal",
        "alexdunne",
        "swissmanu",
        "bm-software",
        "mvdam",
        "esemesek",
        "mrnickel",
        "souvik-ghosh",
        "nossbigg",
        "saranshkataria",
        "tykus160",
        "jakebloom",
        "ceyhun",
        "mcmar",
        "theohdv",
        "romain-faust",
        "bebebebebe",
        "Naturalclar",
        "chinesedfan",
        "vtolochk",
        "SychevSP",
        "RageBill",
        "sasurau4",
        "256hz",
        "doumart",
        "drmas",
        "jeremybarbet",
        "ds300",
        "natsathorn",
        "connectdotz",
        "alexeymolchan",
        "alexbrazier",
        "kuasha420",
        "phvillegas",
        "eps1lon",
        "ZihanChen-MSFT"
      ],
      "addedOwners": [
        "kelset"
      ],
      "deletedOwners": [],
      "popularityLevel": "Popular"
    }
  ],
  "reviews": [
    {
      "type": "approved",
      "reviewer": "sandersn",
      "date": "2022-07-11T16:43:42.000Z",
      "isMaintainer": true
    },
    {
      "type": "approved",
      "reviewer": "jablko",
      "date": "2022-07-04T17:27:28.000Z",
      "isMaintainer": false
    }
  ],
  "mainBotCommentID": 1173950362,
  "ciResult": "fail",
  "ciUrl": "https://github.com/DefinitelyTyped/DefinitelyTyped/commit/16e5f757cdec529e237c25e3931438c87ec39c51/checks?check_suite_id=7210016572"
}

@typescript-bot typescript-bot added Edits Owners This PR adds or removes owners Popular package This PR affects a popular package (as counted by NPM download counts). Untested Change This PR does not touch tests labels Jul 4, 2022
@typescript-bot typescript-bot added this to Waiting for Code Reviews in New Pull Request Status Board Jul 4, 2022
@iRoachie
Copy link
Contributor

iRoachie commented Jul 4, 2022

If I understand you correctly, would that mean if react releases a new version (react 19) that has breaking changes to the API we would then have to come back here and specify the "paths" field in the current tsconfig.json?

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

@kelset 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.

@kelset
Copy link
Contributor Author

kelset commented Jul 4, 2022

If I understand you correctly, would that mean if react releases a new version (react 19) that has breaking changes to the API we would then have to come back here and specify the "paths" field in the current tsconfig.json?

yeah - that's my understanding of how DT handles direct dependencies between types (see the links in the body)

@kelset
Copy link
Contributor Author

kelset commented Jul 4, 2022

About the CI error:

Error in styled-components-react-native
Error: 
A module look-up failed, this often occurs when you need to run `npm install` on a dependent module before you can lint.
Before you debug, first try running:
   npm install --prefix /home/runner/work/DefinitelyTyped/DefinitelyTyped/types/styled-components
Then re-run. Full error logs are below.
Errors in typescript@4.8 for external dependencies:
Error: ../styled-components/index.d.ts(21,22): error TS2307: Cannot find module 'csstype' or its corresponding type declarations.
Error: ../styled-components/index.d.ts(29,18): error TS2430: Interface 'CSSObject' incorrectly extends interface 'CSSPseudos'.

This also happened to me locally but running npm install --prefix /home/runner/work/DefinitelyTyped/DefinitelyTyped/types/styled-components addressed it, I'm not sure what I'm supposed to do here on CI :(

@kelset
Copy link
Contributor Author

kelset commented Jul 4, 2022

I tried digging a bit more into styled-components-react-native; it looks like it has created some problems in the past.

Here are some key info:

sooooo yeah. I'm not really sure what I should try to do - @sandersn any ideas? 🤔

@eps1lon
Copy link
Collaborator

eps1lon commented Jul 4, 2022

I tried this in the past as well but didn't succeed. Can't remember in which PR though.

@jablko
Copy link
Contributor

jablko commented Jul 4, 2022

@kelset Thanks for the PR! I think this is the problem with the CI: microsoft/DefinitelyTyped-tools#484 (comment)

styled-components-react-native depends on styled-components, declares styled-components/native, the DT infrastructure currently incorrectly omits the dependency, so styled-components' transitive external dependencies (CSSType) aren't installed.

You'll need some maintainer assistance, I think, either to land microsoft/DefinitelyTyped-tools#484 (comment) (which will clear up the CI), or manually merge this PR in the meantime.

@kelset
Copy link
Contributor Author

kelset commented Jul 4, 2022

Thanks for the insights @jablko! Since today is 4th of July it's likely the maintainers are OOF, but hopefully by end of the week we'll have some more clarity :)

Copy link
Contributor

@jablko jablko left a comment

Choose a reason for hiding this comment

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

Although I'm unfamiliar with these types, I confirm the added path mappings will make @types/react-native@0.63 depend on @types/react@16, etc. https://github.com/microsoft/DefinitelyTyped-tools/blob/068213db7286da860ce4d1d71e53ad0fc011fd24/packages/definitions-parser/src/lib/definition-parser.ts#L443

I also followed the links you provided to confirm >=0.69 supports React 18 and >=0.64 supports React 17.

Thanks! 👍

@typescript-bot typescript-bot added the Other Approved This PR was reviewed and signed-off by a community member. label Jul 4, 2022
@kuasha420
Copy link
Contributor

@kelset LGTM! Will this makes the "resolution" field on community typescript template redundant?

@kelset
Copy link
Contributor Author

kelset commented Jul 6, 2022

@kelset Lorenzo Sciandra FTE LGTM! Will this makes the "resolution" field on community typescript template redundant?

probably - I didn't know the TS template had a resolution

@sandersn sandersn merged commit 8999c1c into DefinitelyTyped:master Jul 11, 2022
@typescript-bot typescript-bot removed this from Needs Author Action in New Pull Request Status Board Jul 11, 2022
@kelset kelset deleted the fix-rn-react-dep branch July 11, 2022 17:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Edits Owners This PR adds or removes owners Maintainer Approved Other Approved This PR was reviewed and signed-off by a community member. Popular package This PR affects a popular package (as counted by NPM download counts). The CI failed When GH Actions fails Untested Change This PR does not touch tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants