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

Fixes issue #9 for server-side rendering #10

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

hampusohlsson
Copy link

@hampusohlsson hampusohlsson commented Aug 30, 2016

This should fix issue #9

@sashashakun
Copy link

Fix this issue in my fork https://github.com/sashashakun/ssr-scroll-to

@cusspvz
Copy link

cusspvz commented Nov 14, 2016

In fact, you should NOT load this on server side. What do you need for this on the server-side?

@hampusohlsson
Copy link
Author

@cusspvz have you never heard about server side rendering of react code? It's a way for pages to load faster and with better seo for example

@cusspvz
Copy link

cusspvz commented Nov 14, 2016

I know what SSR is, and I'm asking you why you need it on SSR.

You see, on the Server side, you won't be messing with the dom, neither with a window, since SSR doesn't require an headless browser. So, simply use require('scroll-to') when you need it, such as in a component mount or mouse event.

@cusspvz
Copy link

cusspvz commented Nov 14, 2016

Basically what I'm telling you is do not bring up this repo to your server code, it will only waste your server's resources because it will not be used.

The SSR "fix" should be made at your code base instead of this repo.

@hampusohlsson
Copy link
Author

Sure, that is one option, but I'm using ES6 import syntax, which has to be used in the top of the file. Plus I am hesitant to introducing such workarounds

@hampusohlsson
Copy link
Author

But I respect your opinion. Will look for an alternative library

@cusspvz
Copy link

cusspvz commented Nov 14, 2016

Please note, that I'm not affiliated with scroll-to just discovered this lib today and opted for making my own scrollTo code instead.

I've just commented because I think this PR shouldn't be landed. If you replace the dependencies with noop functions, the module won't work.

Usually in isomorphic scenarios, there are errors because of plugins that could be used on the server-side, but they are calling for the global window or any other one that is not present on the node side.

For those cases, the module should be edited in favor of compatibility to node environment. But I don't think this is the case. This module is useless on the server side because it relies entirely on the dom (the only way to make it useful is to emerge it on a dom-compatible environment, which could also be made on node without modifying this).

So the last option is to add the compatibility on your code base.

Sure, that is one option, but I'm using ES6 import syntax.

ES6 is not ES5 stricted, so you will not have any problems on having a require() statement wrapped with a conditional.

PS: Isomorphism/Universalism brings up some scenarios where you do have to do wrappers. Specially if you use your frontend codebase building into more outputs beyond browser and node such as: cordova, electron and so on...

Example of mylocalforage wrapper approach.
captura de ecra 2016-11-14 as 16 31 15

Hope it helped.

@cusspvz
Copy link

cusspvz commented Nov 14, 2016

Give a look at scroll

@millermatt
Copy link

This issue causes problems when using Jest to test React components.

  ● Test suite failed to run

    Cannot find module 'tween' from 'index.js'

      at Resolver.resolveModule (node_modules/jest-resolve/build/index.js:169:17)
      at Object.<anonymous> (node_modules/scroll-to/index.js:5:13)

Please merge the PR.

@notatestuser
Copy link

Yeah, that ^

@ztanner
Copy link

ztanner commented Jul 18, 2017

If you're using Jest, you can just mock it until this gets merged. ie:

jest.mock('scroll-to-element', () => 'scroll-to-element');

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.

6 participants