search

yaodingyd / react-flickity-component

A React.js component for using @desandro's Flickity
314 stars 51 forks source link

fix server side rendering bug #71

Closed ebrahimiaval closed 5 years ago

ebrahimiaval commented 5 years ago

fix "ReferenceError: window is not defined" error on SSR project. in server side 'winodw' object does not exist. we can import 'flickity' package when we need to render slider in client and in server generate pure DOM.

yaodingyd commented 5 years ago

This doesn't look right. Besides we already have the check in componentDidMount lifecycle.

yaodingyd commented 5 years ago

NO, This is right. in SSR componentDidMount do not effect JUST 'constructor' and 'render' work. you must ignore import window dependency plugins to fix 'window undefined' error. in this commit i ignored flickity to fix it. now we can use in both side.

So in SSR componentDidMount is not called. But the only usage of Flickity is in componentDidMount where it initializes. So what good it does to set to undefined since it's never called?

Can you make sure your SSR window reference is in this lib?

ebrahimiaval commented 5 years ago

NO, This is right. in SSR componentDidMount do not effect JUST 'constructor' and 'render' work.

you must ignore import window dependency plugins to fix 'window undefined' error. in this commit i ignored flickity to fix it. now we can use in both side.

please test it on real SSR project to see error.

yaodingyd commented 5 years ago

Yes, I agree with you on that in SSR componentDidMount does not take effect. My point is Flickity is actually called in componentDidMount, so since this lifecycle is never invoked, how come setting it to undefined solved the issue?

If you want me to test on a real SSR project, can you create a minimum repo to reproduce your issue?

ebrahimiaval commented 5 years ago

Yes, I agree with you on that in SSR componentDidMount does not take effect. My point is Flickity is actually called in componentDidMount, so since this lifecycle is never invoked, how come setting it to undefined solved the issue?

If you want me to test on a real SSR project, can you create a minimum repo to reproduce your issue?

yon can clone my RSSR-dev project and insert your flickity sample in to App.js then run 'npm i' and 'npm run dev' to can test it

andyfangaf commented 5 years ago

Can confirm this is happening for me on SSR. Any updates on merging this @yaodingyd ?

theolampert commented 5 years ago

I can reproduce @ebrahimiaval's issue. I've never had a need for the package server side so don't know how it works but have received a few PRs related to it.

Ultimately it would be great if this was handled upstream since it would fix the issue for others too and not just end uses of this library, which is intended to just be a wrapper for using flickity in react. But it doesn't look like anyone wants to pick this up right now https://github.com/metafizzy/flickity/issues/353.

A check like the one proposed might be good enough for now?

yaodingyd commented 5 years ago

I'll take another look tonight

yaodingyd commented 5 years ago

I would say this should be handled by developers in their own build process. We have PR fix other SSR issues from people and SSR have been working for them, given they probably have the global.window check somewhere. That being said, I'm OK if you @theolampert want to merge this.

theolampert commented 5 years ago

@yaodingyd Yeah I think I'm basically in a agreement but a little on the fence. I like the idea of just being able to install it without any friction even if the behaviour is maybe a little unexpected.

theolampert commented 5 years ago

I believe this is covered by #79 now so I'll close this, thank you for your work @ebrahimiaval