feat: WeakMap when environments are referenced

[maraisr]

With this PR I aim to address the situation where environments are removed, but never garbage collected as references are held onto in this Map.

A simple case could be:

const MyApp = () => {

	const [environment, setEnvironment] = useState(new Environment({ network, store }));

	useEffect(() => {
		setTimeout(() => {
			setEnvironment(new Environment({ network, store }));
		}, 1e3);
	}, []);

	return (<RelayEnvironmentProvider environment={environment}>
		<MyComponentTreeWith_A_useFragment/>
	</RelayEnvironmentProvider>);
};

as you'll find from that example, the dataResources map will be forever growing, with nothing clearing.

fixes: #3013

[sibelius]

It would fix SSR use cases

[alunyov]

Thanks for this change!

[facebook-github-bot]

@jstejada has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[maraisr]

Thanks for that @alunyov, I have made those IMap changes... I'm not the best Flow writer, so please do tell me if there was a better way.

Also; should we try and implement something that mimic's the WeakMap behavior for those targets where WEAKMAP_SUPPORTED is false, or pull in core-js polyfill for that? On that note though, afaik WeakMap is supported for the same browser stack as React - so we could do away with the check? All except Internet Explorer 9 - which need polyfills anyway?

[alunyov]

Thanks for the update!

Also; should we try and implement something that mimic's the WeakMap

IMO, we may just keep it as it is right now. They're just a few instances where we are using WeakMap, so - not a huge overhead. And also pulling in the polyfill will most likely conflict with our internal setup.

[facebook-github-bot]

@alunyov has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[maraisr]

Oh sorry, I may have no articulated myself correctly. My bad. I'm not saying we include the polyfill within this repo. Im merely saying, that bundlers, runtimers etc.. That they include the polyfill, if they choose. Much like how react isnt including a polyfill for Map, but rather saying that if IE9 is your target, then you'll need to include a polyfill for Map.

What I'm really trying to say is, let this library articulate that we want the behavior of a WeakMap by using it - and simply say that for environments where that isn't directly available, that you have to use polyfill's, by whatever means your setup is like.. So Facebook might use rome, babel, whatever.

[facebook-github-bot]

@alunyov merged this pull request in a727f8c.