Skip to content
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

no window on serve side #48

Closed
tcurdt opened this issue Mar 31, 2015 · 7 comments
Closed

no window on serve side #48

tcurdt opened this issue Mar 31, 2015 · 7 comments
Labels

Comments

@tcurdt
Copy link

tcurdt commented Mar 31, 2015

For example

https://github.com/fmoo/react-typeahead/blob/master/src/typeahead/index.js#L5

creates a dependency to window prevents server side rendering as is (unless working around it by declaring it as {}).

@fmoo
Copy link
Owner

fmoo commented Mar 31, 2015

Oh nasty. I think this is some leftover stuff from before we used browserify.

Should be safe to remove. Want to send a PR?

@fmoo fmoo added the bug label Mar 31, 2015
@rhalff
Copy link
Contributor

rhalff commented Apr 9, 2015

Hi, I've run into the same issue.

I've added a pull request #55 to address it.

It was not safe to just remove the check though, because the generated dist file will be unable to find window.React, I've added a transform to the build script which fixes that issue.

This restores the old behavior, there are probably more complete solutions, but this one seems to work for now.

@fmoo
Copy link
Owner

fmoo commented Apr 9, 2015

It was not safe to just remove the check though, because the generated dist file will be unable to find window.React, I've added a transform to the build script which fixes that issue.

Do you know if this was broken for both webpack and browserify? Or just the later?

@rhalff
Copy link
Contributor

rhalff commented Apr 9, 2015

Browserify & webpack will just work they do not need the window.React test.

The examples broke because they rely on window.React.

In short:

  • I've removed all window.React's from the source
  • changed the build script so the dist file does generate a version which tests for window.React

@fmoo fmoo closed this as completed in 486675b Apr 10, 2015
@fmoo
Copy link
Owner

fmoo commented Apr 10, 2015

K, this is merged. Do you need a 1.0.3 release for this? Or is this mainly for source distributions?

@rhalff
Copy link
Contributor

rhalff commented Apr 11, 2015

Thanks, it does need a new release. Projects depending on react-typeahead will then have serverside rendering fixed.

@fmoo
Copy link
Owner

fmoo commented Apr 12, 2015

@rhalff - got it. I cut a 1.0.3 release just now with just this change. Thanks for contributing!

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

No branches or pull requests

3 participants