Storytel / react-redux-spinner

An automatic spinner for react and redux
https://storytel.github.io/react-redux-spinner/
MIT License
81 stars 13 forks source link

Server Side Rendering error #4

Closed ilusharulkov closed 6 years ago

ilusharulkov commented 7 years ago

Hello! There is server side rendering when try to use react-redux-spinner. The problem with window object:

ReferenceError: window is not defined
/Users/admin/mt_front/node_modules/react-redux-spinner/dist/index.js:200
            return /msie [6-9]\b/.test(window.navigator.userAgent.toLowerCase());
Stack:
  at /Users/admin/mt_front/node_modules/react-redux-spinner/dist/index.js:200:31
    at memoize (/Users/admin/mt_front/node_modules/react-redux-spinner/dist/index.js:195:48)
    at module.exports (/Users/admin/mt_front/node_modules/react-redux-spinner/dist/index.js:217:69)
    at Object.<anonymous> (/Users/admin/mt_front/node_modules/react-redux-spinner/dist/index.js:97:37)
    at __webpack_require__ (/Users/admin/mt_front/node_modules/react-redux-spinner/dist/index.js:30:30)
    at Object.<anonymous> (/Users/admin/mt_front/node_modules/react-redux-spinner/dist/index.js:71:2)
    at __webpack_require__ (/Users/admin/mt_front/node_modules/react-redux-spinner/dist/index.js:30:30)
    at Object.defineProperty.value (/Users/admin/mt_front/node_modules/react-redux-spinner/dist/index.js:57:19)
    at __webpack_require__ (/Users/admin/mt_front/node_modules/react-redux-spinner/dist/index.js:30:30)
    at /Users/admin/mt_front/node_modules/react-redux-spinner/dist/index.js:50:18
csainty commented 7 years ago

We are not doing any server-side rendering for me to test with quickly. In this case I expect we can just shortcut the test. But without a test environment I can't be sure that is the only fix needed.

matiishyn commented 7 years ago

I have the same issue

danieleleoni commented 7 years ago

Same issue for me as well!

noseglid commented 7 years ago

We're not doing any server-side rendering at the moment.

We'd be happy to accept pull requests to get this working!

noseglid commented 7 years ago

This may be fixed with v0.6.0. We're now using webpack to export an UMD bundle. Anyone doing server-side rendering willing to try? It's published to npm.

sthzg commented 7 years ago

I had a bit of time to inspect this and the offending code results from building Webpack's style-loader into the final release bundle (there are possibly more dependencies with unsafe access of document or window properties). I am not sure how much time I can get to inspect. If it is enough to come up with useful information I will update this issue.

sthzg commented 6 years ago

One possible way around it is requiring the css only in browser environments: https://github.com/sthzg/react-redux-spinner/commit/807ac609acc8d436ed193b930d853f8d60367f4b

This introduces one dependency (exenv) which has a minimal footprint. To avoid the dependency, the canUseDOM check could easily be ported to this project.

var canUseDOM = !!(
  typeof window !== 'undefined' &&
  window.document &&
  window.document.createElement
);

Another way to solve the problem could be using universal style loader like https://github.com/kriasoft/isomorphic-style-loader. Could be a little more overhead though.

Would be awesome to craft an MR for this, so I'd be happy to hear your feedback and find a solution for SSR.

noseglid commented 6 years ago

Thank you for investigating. I believe the best course of action would be to simply not include the css in the bundle. This would have multiple benefits such as working for SSR, reducing bundle size and simplifying build steps.

Of course the drawback is that any integration would need to manually include the css - but this is how all major frameworks which include css does it (e.g. bootstrap, foundation, react-md etc).

I'll make the necessary changes and see what it will look like!

sthzg commented 6 years ago

Thanks for taking the time. I totally agree that not bundling the css is the most elegant solution. Looking forward to the update.

noseglid commented 6 years ago

If you want to test it, you could check out the branch no-css-in-bundle, run yarn then yarn build (this will create dist/react-redux-spinner.js and dist/react-redux-spinner.css).

Now running yarn link in the react-redux-spinner. Then run yarn link react-redux-spinner in your project and it "should" be the same as if you installed it from npm

sthzg commented 6 years ago

Thanks @noseglid. I didn't get to it today, but I will test the branch tomorrow morning and report back to you.

sthzg commented 6 years ago

Sorry that it took me longer than promised to respond.

The no-css-in-bundle branch works as expected, except that I ran into a problem after the commit that refactors prop-types to an external. It leads to a Can't resolve 'PropTypes' error, although the peer dependency in our host project is fulfilled. I need to check whether it is something related to our Webpack config.

noseglid commented 6 years ago

No worries. I appreciate your help!

I yarn gets a bit weirded out when linking and externals. I'm fairly certain it wont be an issue when installed "for real" meaning it will be in the same dependency structure.

There was an issue how externals were defined. Fixed in v1.0.1

I'll make a release out of this!