@Hotell Thank you for submitting this PR!

🔔 @me @Kovensky @xbIm @ngbrown @theigor @alitaheri @herrmanno @DaIgeb @allienna @schlesingermatthias @InsidersByte @artyomsv @dan-j @minodisk @samwalshnz @coding2012 @Aaronphy @khanhas @denistirilis @mantasmarcinkus @mattoni @paustint @j-fro @endigo @walkerburgin @vsiao @danilojrr @Batbold-Gansukh @octatone @chengsieuly @mretolaza @katbusch @vitosamson @LKay @aaronbeall @jrakotoharisoa @ianks @kandros @radziksh @Kimahriman @santiagodoldan @alaatm @SupernaviX @KieranPeat @martinnov92 @junbong @uncovertruth @MartynasZilinskas @theruther4d @asvetliakov @abirkholz @ZheyangSong @andrewhathaway @grzesie2k @KostyaEsmukov @GiedriusGrabauskas @chnoch @apare @johnnyreilly - 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.

If no reviewer appears after a week, a DefinitelyTyped maintainer will review the PR instead.

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

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

If you bump a definition to a newer TypeScript version, all definitions depending on it need to be upgraded too.
Yeah I know, in the case of React that's a lot...

@tkrotoff huh I don't think that's good idea :D ... so maybe we can omit that dts-lint assertion on empty render() ?

With that applied:

• TS 2.6 users would get no compile error for render(){ }
• TS >=2.7 would get proper compile error

WDYT ?

@Hotell I haven't seen anything from you in a while and this PR currently has problems that prevent it from being merged. The PR will be closed tomorrow if there aren't new commits to fix the issues.

@Kovensky

While at it, it could be good to make state optional (readonly state?: Readonly<S>); as using state without initializing it first is an error.

I think that belongs to separate PR

• Also I would update state and props to readonly, because they are readonly in runtime

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

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

So now I've down graded TS back to 2.6. There are still a lot of errors because various other 3rd party ambient types as they don't implement render... I'm not sure what's the best practice here... should I go through whole repo and fix every 3rd party type definition that uses react ? :-/

We would have no choice but to fix all of the dependent type defs if we wanted to turn render abstract, I will help take a look at the v2.6 error you found in the dts-lint tests.

We would have no choice but to fix all of the dependent type defs if we wanted to turn render abstract

right, I will have to write some script with codemod..., doing it by hand sounds like a lot of unnecessary work. If anyone knows about some existing solution/lib to write this kind of codemods I'm all ears

@Hotell 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!

@Hotell - It appears Travis did not correctly run on this PR! /cc @RyanCavanaugh to investigate and advise.

I've udpated whole definitelyTyped repo ( all react depended type definitiopns), although Travis is still failing. It would be nice to resolve this soon and get this merged. Updating 5000+ files is not fun....

cc @mhegazy @sandersn @DanielRosenwasser

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

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

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

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

ping @RyanCavanaugh @johnnyreilly

I'm hesitant to merge because Travis is failing...

I know right ! can't you somehow "reboot the build" ? :)

Rebooted. Lots of these errors: * Duplicate Project Name descriptions

hmm I wonder what those errors come from/what does they mean 😫

any ides @RyanCavanaugh @sandersn ?

The change touches a lot of files, and the tester is failing processing the diffs.

Why are we making this change? this seems to be a lot of churn for very little value..

While I know the change is correct, I think @mhegazy has a good intuition here; the pain may not be worth the benefit. Ideally render would have been abstract from the start, but we have a few years worth of React consumers, and React is one of the most-popular .d.ts libraries on DefinitelyTyped.

Yeah I rather agree with @DanielRosenwasser - the change is correct, but I don't feel it gains us a great deal...

I've checkout branch mh/react-abstract-render.

In order to fix stdout maxBuffer exceeded, go to DefinitelyTyped/node_modules/types-publisher/bin/util/util.js and add maxBuffer option to line 159:

child_process_1.exec(cmd, { encoding: "utf8", cwd, maxBuffer: 1024 * 1024 * 1 /* Max: 1 MiB */ }, ...

Documentation here: https://nodejs.org/api/child_process.html#child_process_child_process_exec_command_options_callback
A better fix would be to use spawn instead of exec

btw, git clone DT needs to done with DefinitelyTyped path (default path). If you try another name like DefinitelyTyped-PR-react-abstract => it fails. This should be fixed too, I'm pretty sure it has bitten few DT contributors.

Once maxBuffer has been increased, it takes quite some time but the process works.
Problem, there are a lot of:

• semicolon Missing semicolon
• Non-abstract class 'XXX' does not implement inherited abstract member 'render' from class ...

The semicolon issue I think comes from the addition of stricter TSLint rules without the types being modified after (tell me if I'm wrong - new rules does not apply to untouched types).

I'm in favor for this PR: if someone issues a PR with improvements, we should support him. @Hotell put a lot of work in this, let's fix the "platform" to help him (it will eventually also help future PRs).

Here the output from npm run test: https://privatebin.net/?8f9b087adb79df4e#3m9sGYVrhDWnc/QcLXo3VzsirzXKaLsLxqtQqZmNVVA=

I'm in favor for this PR: if someone issues a PR with improvements, we should support him. @Hotell put a lot of work in this, let's fix the "platform" to help him (it will eventually also help future PRs).

That's a fairly convincing argument @tkrotoff 😄

Would you like to apply your suggested changes to fix the platform? They sound like improvements DT would feel the benefit of...

@tkrotoff the test issue is rather orthogonal. we should fix it to avoid it happening again. A PR to types-publisher would be appreciated.

Now for the current PR. I still believe the value added by this change does not warrant the cost (breaking changes to react users).

I still believe the value added by this change does not warrant the cost (breaking changes to react users).

Hey @mhegazy, as far as I know this PR should not cause breaking changes to React users. The only users who would be broken are people that have components which lack a render method. That's not valid code; it would be an error at runtime. Anyone broken by this would arguably be super grateful!

@Hotell has another PR which does represent breaking changes to React users which I'm more hesitant to recommend. But if @tkrotoff's changes mean that the tests pass successfully I can't think of any reason this PR shouldn't get merged.

It's possible I've missed something you've picked up on, is there any other reason you can think of that this shouldn't get merged assuming tests pass?

@Hotell To keep things tidy, we have to close PRs that aren't mergeable but don't have activity from their author. No worries, though - please open a new PR if you'd like to continue with this change. Thank you!

Hey Im on vacation so afk sorry for no response, sad to see this closed though...
@tkrotoff huge, huge thanks for finding the solution with the build etc ♥️👌 !!! You rock dude.

@mhegazy, @DanielRosenwasser

What @johnnyreilly said. If someone's is missing render -> his app will throw on runtime anyway. Only potential "breaking change" could be for js libs authors that have manually written .d.ts ( all they need to do is write down render, 10 seconds effort ). I wanna start working on some codemods for things like this as definitely typed doesn't have semver in it's dictionary 😪
What would be also beneficial for anyone is to have some standard tooling for writing those codemods as I mentioned in #26847

types-publisher PR for maxBuffer: microsoft/types-publisher#471

bad @typescript-bot

What surprised me is that making render() abstract, which it clearly should be, broke all definitions that refer to Component because they must define render(), but the added definitions for render() are pretty useless. I guess there's no way to say that Component/render() is only required by implementations, not for definitions. Unfortunately this does make it a breaking change for any definition not hosted by DT. Hopefully that's not a lot, but I have I no idea... I know I have a handful of my own custom definitions that will break (though I'm personally 100% fine with that)... just rambling, I do wish render had been abstract from the start but having a hard time seeing the justification for the change now. :/