Removed _debugID field from Fiber - Issue #21558

[Pulkit3234]

Removed _debugID field from Fiber

Resolves #21558

[sizebot]

Comparing: 0ae5290...801f85f

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name	+/-	Base	Current	+/- gzip	Base gzip	Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js	=	126.00 kB	126.00 kB	=	40.41 kB	40.41 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js	=	128.82 kB	128.82 kB	=	41.35 kB	41.35 kB
facebook-www/ReactDOM-prod.classic.js	=	406.09 kB	406.09 kB	=	75.11 kB	75.11 kB
facebook-www/ReactDOM-prod.modern.js	=	394.46 kB	394.46 kB	=	73.29 kB	73.29 kB
facebook-www/ReactDOMForked-prod.classic.js	=	406.09 kB	406.09 kB	=	75.11 kB	75.11 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against 801f85f

[bvaughn]

Hey @Pulkit3234! Nice work!

CI is failing for a couple of reasons though. Should be easy to fix:

First, lint/styling. To fix this, just run:

yarn prettier

Then you can commit the minor formatting changes (mostly just spacing, I think).

Secondly, the debugCounter variable in ReactFiber.new.js (and ReactFiber.old.js) is no longer used, so you can delete it.

Lastly a couple of tests are now failing CI. This is a little bit more tricky I think. These tests verified that we deduplicate warnings so they're only shown once per component type. Some of these warnings (like "Function components cannot be given ref" for example) do their de-duplicating using fiber.type.displayName || fiber.type.name || fiber._debugID || '' so if a component doesn't have a name (if it's anonymous) we'll no longer log a unique warning for it.

I think that's probably okay to be honest, but maybe someone else might disagree. Let's fix the first two things above first? :)

[Pulkit3234]

Yes @bvaughn sir, I am fixing the first two things that you told :), But its showing almost 1k+ problems in terminal, I think they are causing failure of tests.

[bvaughn]

CI is still failing b'c of formatting:
https://app.circleci.com/pipelines/github/facebook/react/14042/workflows/63b2ef2f-d01a-46ed-b670-e75d8311929a/jobs/321219

Try running:

yarn prettier-all

Only a single test is failing:

react/packages/react-dom/src/__tests__/ReactFunctionComponent-test.js

Lines 245 to 309 in bb1c821

	it('deduplicates ref warnings based on element or owner', () => {
	// When owner uses JSX, we can use exact line location to dedupe warnings
	class AnonymousParentUsingJSX extends React.Component {
	render() {
	return <FunctionComponent name="A" ref={() => {}} />;
	}
	}
	Object.defineProperty(AnonymousParentUsingJSX, 'name', {value: undefined});
	let instance1;
	expect(() => {
	instance1 = ReactTestUtils.renderIntoDocument(
	<AnonymousParentUsingJSX />,
	);
	}).toErrorDev('Warning: Function components cannot be given refs.');
	// Should be deduped (offending element is on the same line):
	instance1.forceUpdate();
	// Should also be deduped (offending element is on the same line):
	ReactTestUtils.renderIntoDocument(<AnonymousParentUsingJSX />);
	// When owner doesn't use JSX, and is anonymous, we warn once per internal instance.
	class AnonymousParentNotUsingJSX extends React.Component {
	render() {
	return React.createElement(FunctionComponent, {
	name: 'A',
	ref: () => {},
	});
	}
	}
	Object.defineProperty(AnonymousParentNotUsingJSX, 'name', {
	value: undefined,
	});
	let instance2;
	expect(() => {
	instance2 = ReactTestUtils.renderIntoDocument(
	<AnonymousParentNotUsingJSX />,
	);
	}).toErrorDev('Warning: Function components cannot be given refs.');
	// Should be deduped (same internal instance, no additional warnings)
	instance2.forceUpdate();
	// Could not be deduped (different internal instance):
	expect(() =>
	ReactTestUtils.renderIntoDocument(<AnonymousParentNotUsingJSX />),
	).toErrorDev('Warning: Function components cannot be given refs.');
	// When owner doesn't use JSX, but is named, we warn once per owner name
	class NamedParentNotUsingJSX extends React.Component {
	render() {
	return React.createElement(FunctionComponent, {
	name: 'A',
	ref: () => {},
	});
	}
	}
	let instance3;
	expect(() => {
	instance3 = ReactTestUtils.renderIntoDocument(<NamedParentNotUsingJSX />);
	}).toErrorDev('Warning: Function components cannot be given refs.');
	// Should be deduped (same owner name, no additional warnings):
	instance3.forceUpdate();
	// Should also be deduped (same owner name, no additional warnings):
	ReactTestUtils.renderIntoDocument(<NamedParentNotUsingJSX />);
	});

You can change this test from it(...) to xit(...) for now to disable it. Then I can raise this discussion point with the team to see if there are concerns.

[Pulkit3234]

Thanks a lot @bvaughn sir for guiding and correcting me while I was working on this issue 😄. I can't tell at this moment that how much delighted I am as its the first successful pr that I made on my favorite Javascript library React. I am new to open source and I think I have learnt so much working on this single issue, but I wanted to know that how do all the react maintainers like you and Dan sir add new features or improve the existing ones, like how do you notice bugs in such a huge codebase?

[bvaughn]

I think this change is fine, but I'm going to run it by the team and see if anyone else thinks the downside (DEV warning de-duping) is worrisome.

[bvaughn]

I'm going to go ahead and land this change 👍🏼 Thanks!

[Pulkit3234]

@bvaughn sir, thank you so much for mentioning me as well in the final commits, that means a lot 😄. Looking forward to make some awesome contributions to react in future!, with your help and guidance!