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

fix: crash outside browser environment #377

Merged
merged 6 commits into from
Nov 25, 2019
Merged

Conversation

VadimKorobka
Copy link
Contributor

No description provided.

@VadimKorobka
Copy link
Contributor Author

@supasate Pls, check and bump version, it's very hot changes.

src/reducer.js Outdated Show resolved Hide resolved
src/reducer.js Outdated Show resolved Hide resolved
@codejockie
Copy link
Contributor

#376 Fix

@CoericK
Copy link

CoericK commented Nov 19, 2019

@codejockie any update on this, when is this going to be merge and released?

@rmunson
Copy link

rmunson commented Nov 19, 2019

This eliminates valid server-side handling of initial router dispatches in connected redux stores. It was previously possible to process any valid passed Location instance server-side without the need to short-circuit the reducer. It’d be a more complete fix to remove the unnecessary check against window.location.search as fallback logic from the initial location.search access. The location object passed into the reducer factory function should be sufficient.
Onus should be on the implementing user the provide the proper search value rather than assuming a window object is present.

@codejockie
Copy link
Contributor

@KorobkaVadim could you update this PR according to what @rmunson said above?

@VadimKorobka
Copy link
Contributor Author

Yeah. You are right, I saw this, but these are not my changes. This PR was a hotfix, but something is getting cold. I will fix it in two hours.Thank you!

@VadimKorobka
Copy link
Contributor Author

@codejockie @rmunson Done, pls check

src/reducer.js Outdated Show resolved Hide resolved
Copy link

@rmunson rmunson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me. I appreciate the adjustment.
Should still be functional minus the fallback.

@VadimKorobka
Copy link
Contributor Author

@supasate Pls, check this PR, because a lot of people wait this fix. @codejockie and @rmunson approved these changes.

@dptoot
Copy link

dptoot commented Nov 22, 2019

Is there any movement on this PR being released?. As a server rendered application we are basically locked down until this fix is out in the wild. Thanks so much!

@supasate
Copy link
Owner

LGTM. Appreciate everyone for the hot fix. I'm releasing it soon.

By the way, it may be a good idea to have a test for server side environment to prevent any future break. Appreciate any contribution in advanced!

@supasate supasate merged commit bf700af into supasate:master Nov 25, 2019
supasate added a commit that referenced this pull request Feb 9, 2020
* 'master' of github.com:supasate/connected-react-router:
  fix: crash outside browser environment (#377)
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.

7 participants