SSR should not warn about onscroll

[jimfb]

Ref: #631 (comment)

It's a little surprising that the codepath is even being hit, maybe that's a bug too, but there should clearly be some sort of if check in that area.

[aweary]

@jimfb how do you guys check for SSR in the React source? Just check for a window object or what?

[zpao]

We have a ReactServerRenderingTransaction type that gets used instead of the regular ReactReconcileTransaction.

[aweary]

So this would just entail adding a check to the warning invariant verifying the transaction argument is not ReactServerRenderingTransaction at src/renderers/dom/shared/ReactDOMComponent.js#L212-L220?

[zpao]

Could be that simple. You might have some issues checking types since
there's pooling on these transactions and instanceOf might not work (I
don't recall)

On Wed, Apr 27, 2016 at 11:20 AM, Brandon Dail notifications@github.com
wrote:

So this would just adding a check to the warning invariant checking if
the transaction argument is not ReactServerRenderingTransaction at
src/renderers/dom/shared/ReactDOMComponent.js#L212-L220

react/src/renderers/dom/shared/ReactDOMComponent.js

Lines 212 to 220 in 76a4c46

	function enqueuePutListener(inst, registrationName, listener, transaction) {
	if (__DEV__) {
	// IE8 has no API for event capturing and the `onScroll` event doesn't
	// bubble.
	warning(
	registrationName !== 'onScroll' || isEventSupported('scroll', true),
	'This browser doesn\'t support the `onScroll` event'
	);
	}

?

—
You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#6626 (comment)

[RaitoBezarius]

@zpao Could I give it a try?

[zpao]

It sounds like @aweary might be giving it a try. If not, then you are more then welcome @RaitoBezarius

[aweary]

@RaitoBezarius @zpao I do have a branch setup on my fork for this, was hoping to give it a shot. If you have any insights into how you'd approach it @RaitoBezarius I'd be happy to hear them.

[RaitoBezarius]

@aweary @zpao Correct me if I'm wrong, but I think the solution is just to return earlier in the enqueuePutListener (implemented in this commit: RaitoBezarius@a2f5c59)
But it's okay if you want still to submit the pull request, I have added also a test on my branch to make sure there is no regression with this bug.

[aweary]

@RaitoBezarius I think checking the transaction type would be the recommended way of verifying SSR (maybe?), as @zpao mentioned earlier. But if what you have is sufficient for @zpao and @jimfb then its all yours 😄

[RaitoBezarius]

@aweary In which case, doc would be not defined in browser? As far as I understand, doc is a reference to the global document object which is a requirement for the DOM to exist. Thus, it means that we are not SSR-ing.

Checking the transaction type would be another solution but it creates some dependency on the code, I believe which could be arguably bad? It means also that if we are using another strategy for the transaction other than the default one for SSR, the warning will appear again. Maybe, you could try to duck-type the transaction by checking if it behaves like a SSR-one, useCreateElement === false, renderToStaticMarkup === true (but not always). :/

Anyway, I don't know if my idea could get wrong though, so you might be right too! This is my understanding after playing around with the debugger.

[zpao]

I think at this point I would prefer we make use of the canonical source of information at that point, not environment details to. It's an explicit vs implicit difference (and the implicit could be wrong as code changes in the future). If we need we could potentially add more details to the transactions so that checking attributes is safe without adding a dependency for instanceOf.

[aweary]

@zpao would adding a serverTransaction: true property to ReactServerRenderingTransaction be acceptable?

[zpao]

Might want to change the name (I might call it something like isServerRendering), but if that's the approach we need to take then it's doable. I haven't looked at this code to say if that's definitely the right approach, I'll let you tell me if that's necessary :)

[aweary]

@zpao transaction instanceof ReactServerRenderingTransaction does seem to work, so I think it depends on whether we want to rely on the prototype chain of transaction being maintained. If that's something you feel isn't going to change, then it's probably safe to just use instanceof. Otherwise adding isServerRendering is always easy enough :)

[aweary]

@zpao all of that logic in enqueuePutListener related to doc, is that just to check if we're using SSR? If so, would checking for ReactServerRenderingTransactions essentially replace that and let us remove that check?