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 for window undefined error in serverside rendering #1634

Closed
wants to merge 1 commit into from

Conversation

Darrken
Copy link
Contributor

@Darrken Darrken commented Feb 8, 2019

Addresses #1619.

@codecov
Copy link

codecov bot commented Feb 8, 2019

Codecov Report

Merging #1634 into master will decrease coverage by 0.07%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1634      +/-   ##
==========================================
- Coverage   89.76%   89.69%   -0.08%     
==========================================
  Files          16       16              
  Lines        1114     1116       +2     
  Branches      194      194              
==========================================
+ Hits         1000     1001       +1     
- Misses         15       16       +1     
  Partials       99       99
Impacted Files Coverage Δ
src/date_utils.js 98.66% <50%> (-0.44%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 164d10d...31073e4. Read the comment docs.

@julien1619
Copy link

julien1619 commented Feb 20, 2019

Hi! When do you plan to merge this PR? It would be really useful to fix the SSR case with translations enabled. Thanks!

@@ -49,6 +49,10 @@ import isWithinInterval from "date-fns/isWithinInterval";
import toDate from "date-fns/toDate";
import parse from "date-fns/parse";

if (typeof window === 'undefined') {
global.window = {}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be much better to avoid modifying the global environment, and instead guard the problematic access to window with an undefined check. I think it would be best to abstract window away in the places where it's accessed to refer to either window or global(?). Adding a unit test for this would be great as well.

Choose a reason for hiding this comment

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

yes, adding stuff to global is a no-go for libraries like that. ;-)

@stale
Copy link

stale bot commented Aug 30, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Aug 30, 2019
@stale stale bot closed this Sep 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants