-
Notifications
You must be signed in to change notification settings - Fork 30.1k
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
feat(react): Update stable types for v17 #48971
Conversation
@eps1lon Thank you for submitting this PR! This is a live comment which I will keep updated. 4 packages in this PR
Code ReviewsBecause this is a widely-used package, a DT maintainer will need to review it before it can be merged. Status
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": 48971,
"author": "eps1lon",
"headCommitAbbrOid": "ff3e81e",
"headCommitOid": "ff3e81ee5146465089131975734ff31c863f4bd8",
"stalenessInDays": 0,
"lastPushDate": "2020-11-20T14:36:41.000Z",
"reopenedDate": "2020-10-20T23:51:51.000Z",
"lastCommentDate": "2020-11-20T11:38:38.000Z",
"maintainerBlessed": false,
"hasMergeConflict": false,
"isFirstContribution": false,
"popularityLevel": "Critical",
"pkgInfo": [
{
"name": "react-dom",
"kind": "edit",
"files": [
{
"path": "types/react-dom/index.d.ts",
"kind": "definition"
},
{
"path": "types/react-dom/v16/index.d.ts",
"kind": "definition"
},
{
"path": "types/react-dom/v16/node-stream/index.d.ts",
"kind": "definition"
},
{
"path": "types/react-dom/v16/server/index.d.ts",
"kind": "definition"
},
{
"path": "types/react-dom/v16/test-utils/index.d.ts",
"kind": "definition"
},
{
"path": "types/react-dom/v16/test/react-dom-tests.tsx",
"kind": "test"
},
{
"path": "types/react-dom/v16/tsconfig.json",
"kind": "package-meta",
"suspect": "not the required form"
},
{
"path": "types/react-dom/v16/tslint.json",
"kind": "package-meta",
"suspect": "not the required form"
}
],
"owners": [
"MartynasZilinskas",
"theruther4d",
"Jessidhia"
],
"addedOwners": [],
"deletedOwners": [],
"popularityLevel": "Critical"
},
{
"name": "react-is",
"kind": "edit",
"files": [
{
"path": "types/react-is/index.d.ts",
"kind": "definition"
},
{
"path": "types/react-is/v16/index.d.ts",
"kind": "definition"
},
{
"path": "types/react-is/v16/react-is-tests.tsx",
"kind": "test"
},
{
"path": "types/react-is/v16/tsconfig.json",
"kind": "package-meta",
"suspect": "not the required form"
},
{
"path": "types/react-is/v16/tslint.json",
"kind": "package-meta-ok"
}
],
"owners": [
"AviVahl",
"christianchown",
"eps1lon"
],
"addedOwners": [],
"deletedOwners": [],
"popularityLevel": "Popular"
},
{
"name": "react-test-renderer",
"kind": "edit",
"files": [
{
"path": "types/react-test-renderer/index.d.ts",
"kind": "definition"
},
{
"path": "types/react-test-renderer/v16/index.d.ts",
"kind": "definition"
},
{
"path": "types/react-test-renderer/v16/react-test-renderer-tests.ts",
"kind": "test"
},
{
"path": "types/react-test-renderer/v16/shallow/index.d.ts",
"kind": "definition"
},
{
"path": "types/react-test-renderer/v16/tsconfig.json",
"kind": "package-meta",
"suspect": "not the required form"
},
{
"path": "types/react-test-renderer/v16/tslint.json",
"kind": "package-meta",
"suspect": "not the required form"
}
],
"owners": [
"arvitaly",
"lochbrunner",
"johnnyreilly",
"jgoz",
"Jessidhia",
"maddhruv"
],
"addedOwners": [],
"deletedOwners": [],
"popularityLevel": "Critical"
},
{
"name": "react",
"kind": "edit",
"files": [
{
"path": "types/react/index.d.ts",
"kind": "definition"
},
{
"path": "types/react/test/elementAttributes.tsx",
"kind": "test"
},
{
"path": "types/react/v16/OTHER_FILES.txt",
"kind": "package-meta-ok"
},
{
"path": "types/react/v16/global.d.ts",
"kind": "definition"
},
{
"path": "types/react/v16/index.d.ts",
"kind": "definition"
},
{
"path": "types/react/v16/jsx-dev-runtime.d.ts",
"kind": "definition"
},
{
"path": "types/react/v16/jsx-runtime.d.ts",
"kind": "definition"
},
{
"path": "types/react/v16/test/cssProperties.tsx",
"kind": "test"
},
{
"path": "types/react/v16/test/elementAttributes.tsx",
"kind": "test"
},
{
"path": "types/react/v16/test/hooks.tsx",
"kind": "test"
},
{
"path": "types/react/v16/test/index.ts",
"kind": "test"
},
{
"path": "types/react/v16/test/managedAttributes.tsx",
"kind": "test"
},
{
"path": "types/react/v16/test/tsx.tsx",
"kind": "test"
},
{
"path": "types/react/v16/tsconfig.json",
"kind": "package-meta",
"suspect": "not the required form"
},
{
"path": "types/react/v16/tslint.json",
"kind": "package-meta",
"suspect": "not the required form"
}
],
"owners": [
"johnnyreilly",
"bbenezech",
"pzavolinsky",
"digiguru",
"ericanderson",
"DovydasNavickas",
"theruther4d",
"guilhermehubner",
"ferdaber",
"jrakotoharisoa",
"pascaloliv",
"hotell",
"franklixuefei",
"Jessidhia",
"saranshkataria",
"lukyth",
"eps1lon",
"zieka",
"dancerphil",
"dimitropoulos",
"disjukr",
"vhfmag",
"hellatan"
],
"addedOwners": [],
"deletedOwners": [],
"popularityLevel": "Critical"
}
],
"reviews": [
{
"type": "approved",
"reviewer": "ferdaber",
"date": "2020-11-20T16:49:12.000Z",
"isMaintainer": false
},
{
"type": "stale",
"reviewer": "Andarist",
"date": "2020-11-20T11:24:15.000Z",
"abbrOid": "87585b4"
},
{
"type": "stale",
"reviewer": "n1ru4l",
"date": "2020-11-20T10:08:12.000Z",
"abbrOid": "df6978a"
},
{
"type": "stale",
"reviewer": "maraisr",
"date": "2020-10-24T04:47:45.000Z",
"abbrOid": "3d3fb93"
}
],
"ciResult": "pass"
} |
🔔 @MartynasZilinskas @theruther4d @Jessidhia @AviVahl @christianchown @arvitaly @lochbrunner @johnnyreilly @jgoz @maddhruv @bbenezech @pzavolinsky @digiguru @ericanderson @DovydasNavickas @guilhermehubner @ferdaber @jrakotoharisoa @pascaloliv @Hotell @franklixuefei @saranshkataria @lukyth @zieka @dancerphil @dimitropoulos @disjukr @vhfmag @hellatan — please review this PR in the next few days. Be sure to explicitly select |
Inspecting the JavaScript source for this package found some properties that are not in the .d.ts files. react-dom (unpkg)was missing the following properties:
react-is (unpkg)was missing the following properties:
react-test-renderer (unpkg)was missing the following properties:
|
Looks like there's just too much to benchmark. |
I think this PR should take the time to fix a long running issue with the Having an optional |
We've discussed this in #46691 and decided to postpone any type related breaking change to v18 because React core members specifically asked us not to. If you disagree please continue the discussion in the issue. |
Oh, I missed that one but that makes sense. Thanks for the info 👍 |
I never assume something else. What matters is the impact.
Now he is. We'll see if this changes his actions.
If that was bashing then how is bashing me helpful?
Collaborators can which I'm not.
Considering you had no problem coming in and requesting changes I don't see how you're easily scared. Just open an issue, explain your problem and we go from there. But demanding a certain implementation is certainly the more toxic behavior. |
I was trying to stand up for a fellow OSS contributor - because I think he had more right here and I didn't want to stay silent about this. Just trying to bring my 2 cents here so you could maybe see an alternative perspective and reconsider if how you have handled this was appropriate or not. If you have considered the words used by me as bashing to you, please take my sincere apologies. I'm not a native speaker so choosing the appropriate, polite, wording is not my strongest suit. My intention was not to bash you but to stand up for @n1ru4l . |
I definitely assumed knowledge about the contribution workflow. It seemed patronizing to explain what "request changes" means in GitHub. I'll explain this the next time so that we're on the same level. Sorry about that @n1ru4l. I hope this doesn't stop you from opening issues or commenting on PRs in the future. In my particular github "cultural-bubble" it's generally considered rude to approve/reject PRs targeting code that you do not maintain. So if we accuse each other of toxicity we should consider these different backgrounds. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good. We would need to merge in the latest changes in master to get jsx runtime running for the older versions so hopefully no big merge conflicts 🤞
@eps1lon Unfortunately, this pull request currently has a merge conflict 😥. Please update your PR branch to be up-to-date with respect to master. Have a nice day! |
I just published |
I just published |
I just published |
I just published |
I just published |
I just published |
I just published |
I just published |
* feat(react): Update stable types for v17 * Fix new lint rules * Add v16 types * fix malformed json * fix baseUrl * Add globals to v16 * Remove experimental tests * add v16 shallow renderer * Add MVP for new jsx runtime * Just expose the namespace
Closes #43985
Not including updates forExperimental release channel handled in #49152experimental
release channel. Goal is to have a minimal update that can be shipped as soon as possible.Please fill in this template.
npm test YOUR_PACKAGE_NAME
.)npm run lint package-name
(ortsc
if notslint.json
is present).Select one of these and delete the others:
If changing an existing definition:
[ ]If you are making substantial changes, consider adding atslint.json
containing{ "extends": "dtslint/dt.json" }
. If for reason the any rule need to be disabled, disable it for that line using// tslint:disable-next-line [ruleName]
and not for whole package so that the need for disabling can be reviewed.