Skip to content

[fixed] server side rendering for Modal component #719

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

Merged
merged 1 commit into from
May 22, 2015
Merged

[fixed] server side rendering for Modal component #719

merged 1 commit into from
May 22, 2015

Conversation

maxmalov
Copy link
Contributor

Using document reference in the render method will throw an error
on React.renderToString call on the server side.
See #717.

The proper solution is to remove document.querySelector check. Obviously
we cant detect this feature on the server side, besides it leads to
differencies btw server and client side rendering output, so React will warn
about it, ex:

  1. render on the server side will apply classes: modal fade
  2. render on the client side will apply clasess: modal fade in

Also karma test environment is not suitable for testing server side rendering,
so mocha test run againt nodejs was added.

Using document reference in the render method will throw an error
on React.renderToString call on the server side.
See #717.

The proper solution is to remove document.querySelector check. Obviously
we cant detect this feature on the server side, besides it leads to
differencies btw server and client side rendering output, so React will warn
about it, ex:

1. `render` on the server side will apply classes: `modal fade`
2. `render` on the client side will apply clasess: `modal fade in`

Also karma test environment is not suitable for testing server side rendering,
so mocha test run againt nodejs was added.
@mtscout6
Copy link
Member

I'm banging my head against the wall trying to figure out why document.querySelectorAll was introduced in the first place and if it's still needed. It seems to have been introduced in 96d1360 by @stevoland. Everything still seems to work without it now though. I'm wondering if there was something about the code base or even React at the time that necessitated this, whereas now it's not even needed.

I like the idea of testing the components for Server side rendering, though I wonder if there is a way to survey which of the current tests should work in both and then do so with both. Though this can be done later as proposed in #720.

LGTM

Need an additional review by one of the @react-bootstrap/collaborators

@aabenoja
Copy link
Member

👍

aabenoja added a commit that referenced this pull request May 22, 2015
[fixed] server side rendering for Modal component
@aabenoja aabenoja merged commit 50f32fa into react-bootstrap:master May 22, 2015
@taion
Copy link
Member

taion commented May 22, 2015

@mtscout6

It looks like it was due to how FadeMixin was implemented: d3d3ddc#diff-485e50263d15ff215f56744914b84ba9

Looks like the idea was to use FadeMixin#componentDidMount if document.querySelectorAll was available, but not otherwise.

@mtscout6
Copy link
Member

All seemed to work without it, but I only checked in Chrome. @taion Are you concerned with this being removed? It's not clear from your last comment.

@taion
Copy link
Member

taion commented May 22, 2015

I am not. FadeMixin no longer works like that. Just commenting on why that errant check was there in the first place.

@mtscout6
Copy link
Member

Good to know thanks!

aryad14 pushed a commit to aryad14/react-bootstrap that referenced this pull request Oct 11, 2023
* docs: update readme with correct org name

* fix
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants