Clusters are breaking the universal app #1241

lazarljubenovic · 2017-11-22T02:03:08Z

Issue description

Just importing the cluster module is breaking the app.

Steps to reproduce and a minimal demo of the problem

I have a map where all its markers are under a cluster. The map is wrapped into a conditional so it renders only on client (not on the server) because there are known issues. However, it still breaks the server side rendering because it tries to access window at some point. This probably happens during module initialization, since the component itself never got instantiated.

Current behavior

Apps using cluster cannot be used in a server side app.

Expected/desired behavior

Apps should not break.

angular & angular-google-maps version

@angular/* at 5.0.0
@agm/core at 1.0.0-beta.2

Other information

I'm looking for a workaround ATM but, of course, would be nice if users having a Universal app do not have to do anything. We should at least wrap all code with isPlatformBrowser and just list that as a caveat for now, until we figure out how to render maps on the server, if it's even possible.

The text was updated successfully, but these errors were encountered:

sebholstein · 2017-12-02T10:30:46Z

@lazarljubenovic yeah wrapping would be a good solution for now.
cc @jigfox

jigfox · 2017-12-04T06:08:39Z

I will look into this

jigfox · 2017-12-17T20:00:01Z

@lazarljubenovic I don't have any experience with universal apps, and I'm not sure how to use isPlatformBrowser properly, would you be able to give me some pointers?

lazarljubenovic · 2017-12-17T20:10:05Z

@jigfox Sure! You can inject the token PLATFORM_ID into a component, like so:

constructor(@Inject(PLATFORM_ID) private platformId: any) {
}

Then you can use Angular functions such as isPlatformBrowser or isPlatformServer to conditionally run certain pieces of code only on certain platforms.

constructor(@Inject(PLATFORM_ID) private platformId: any) {
  if (isPlatformBrowser(this.platformId)) {
    console.log('This code runs only in browser')
  }
}

jigfox · 2017-12-19T11:02:25Z

I'm not sure where to put this, the call to window is in code provided by js-marker-clusterer:

markerclusterer.js#L698 and markerclusterer.js#L1270 so cluster-manager.ts#L3 should be wrapped?

Maybe it's necessary to disable the whole cluster plugin on not Browser platforms? But maybe a rewrite markerclusterer.js could help, because it could be rewritten without a window reference

lazarljubenovic · 2017-12-19T13:21:57Z

Ah, it's code from third-party lib. Jeez. I wonder if just saying something like this would help:

const oldWindow = window || {}
window = {
  setTimeout: oldWindow.setTimeout
}
import 'js-marker-clusterer'
window = oldWindow

But I think that server will break from just finding window while executing code, since the variable has not been instantiated, so that will probably break too, even if it's a good idea.

Could we do the import not at the beginning of the file, but inside a component? What would that even do?

lazarljubenovic · 2017-12-20T10:44:02Z

By the way, it says on that Google Map repo this:

Please note: This repository is not currently maintained, and is kept for historical purpose only.

So what about we just fork that on our own? Since it's a historical repo, it will never change which means there's no problem just using our own tweaked version which doesn't use window at all?

lazarljubenovic · 2017-12-22T09:22:05Z

Oh, and regarding this:

But I think that server will break from just finding window while executing code, since the variable has not been instantiated, so that will probably break too, even if it's a good idea.

This looks very basic but I've only learned this a few day ago:

If you try doing if (window == null) or similar and window does not exist as a variable in any visited scope, the code will break with a ReferenceError. However, if you do if (typeof window == 'undefined'), it will work perfectly fine and give expected results even if window has never been declared (which is our issue on server).

I guess I could try playing around and forking our own version of the Cluster repo and see what can be done using this.

cmddavid · 2017-12-27T17:06:44Z

I tried fixing this using the following approach:

http://ideasintosoftware.com/typescript-conditional-imports/

But I had no luck, as AOT compiler will start complaining the directive can no longer be found.

lazarljubenovic · 2017-12-27T17:53:10Z

Yeah, AOT compiler statically analyzes the code and thus dynamic imports sure sound like something that's not possible to do in Angular. Have you had time trying to fork the repo and just removing access to `window`? Looks like it's used only in a couple of places that can easily be avoided. On Dec 27, 2017 18:06, "cmddavid" <notifications@github.com> wrote: I tried fixing this using the following approach: http://ideasintosoftware.com/typescript-conditional-imports/ But I had no luck, as AOT compiler will start complaining the directive can no longer be found. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1241 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHTnkXZ85QbMrFT7timh9MPWDeL5piYiks5tEnkmgaJpZM4QmwTi> .

cmddavid · 2017-12-28T00:04:21Z

Not yet, I might try it later this week. I'm not a fan of changing the source repo though, even though it is not being maintained at this moment, it might be later. But I guess we don't have any other option. I like the suggestion to completely remove the window references, if that would be possible that would be great.

lazarljubenovic · 2017-12-28T09:25:20Z

Well, it's been two years since the last update so that's why I think forking in this particular instance, at least as a temporary solution, wouldnt be too bad.

…

On Thu, Dec 28, 2017 at 1:04 AM, cmddavid ***@***.***> wrote: Not yet, I might try it later this week. I'm not a fan of changing the source repo though, even though it is not being maintained at this moment, it might be later. But I guess we don't have any other option. I like the suggestion to completely remove the window references, if that would be possible that would be great. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1241 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHTnkfZdUcmrInBzlNL41wJ6znRzWVCoks5tEtsHgaJpZM4QmwTi> .

mbrezovsky · 2018-01-03T12:37:33Z

I forked js-marker-clusterer project and made the fix. It prevents initialization MarkerClusterer when window is not defined (on server side rendering). So marker clusterer works correctly in both cases now.

It's published in npm. https://www.npmjs.com/package/js-marker-clusterer-universal

js-marker-clusterer dependency should be replaced by js-marker-clusterer-universal to take effect.

cmddavid · 2018-01-07T18:45:08Z

@mbrezovsky nice work, it is working well for me. I did get one error related to @agm/js-marker-clusterer using imports, something that is not allowed yet on the Node.js/Firebase side, so I had to convert the code to ES2015 using Anthony Nahas his solution proposed here: #1052 (comment)

…luster dependency in angular universal build (sebholstein#1241) Replaced js-marker-clusterer dependency to fixed version js-marker-clusterer-universal Fix sebholstein#1241

jonaspraem · 2018-04-17T12:43:39Z

Any updates on this issue?

cmddavid · 2018-04-17T12:52:04Z

@AoschkA I'm still using the workaround as mentioned in my earlier comment. You can find my example here: https://github.com/cmddavid/js-marker-clusterer.git

jonaspraem · 2018-04-17T13:19:46Z

@cmddavid Your readme just explains how to install agm/js-marker-clusterer. How does one integrate this into our own projects?

cmddavid · 2018-04-17T13:47:07Z

@AoschkA, the repo is just the compiled output, I did not any instructions, you can add it to your package.json like this:

"@agm/js-marker-clusterer": "git+https://github.com/cmddavid/js-marker-clusterer.git"

When you install it, you will have the js-marker-clusterer without window is undefined errors, and compiled to ES2015 so it's compatible with services like Firebase.

jonaspraem · 2018-04-17T14:05:42Z

Thx that works. Would be nice if this could be implemented in AGM though. But I appreciate the solution @cmddavid !

maksimbykov · 2018-05-30T17:38:34Z

@cmddavid I made like you said, but there is error ERROR in ./node_modules/@agm/js-marker-clusterer/services/managers/cluster-manager.js
Module not found: Error: Can't resolve 'js-marker-clusterer-universal' in '.\node_modules@agm\js-marker-clusterer\services\managers'

cmddavid · 2018-05-30T18:45:29Z

@maksimbykov can you confirm js-marker-clusterer-universal is in your node_modules folder? perhaps it did not fully install, just in case you could run npm i --save js-marker-clusterer-universal. Also something weird seems to be going on with the file paths there. The first one has a double slash // and the second one misses a backslash \ between node_modules and @agm.

Also worth mentioning I did not check if the package still works with angular 6.

maksimbykov · 2018-05-31T08:22:20Z

@cmddavid thank you very much! probably yesterday I was tired and inattentive

stale · 2018-11-13T19:32:05Z

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.

newprogrammer1991 · 2019-02-12T06:22:44Z

"@agm/js-marker-clusterer": "git+https://github.com/cmddavid/js-marker-clusterer.git" sorry but it doesn't work(don't install anything)

nrozic · 2019-07-08T20:49:34Z

I forked js-marker-clusterer project and made the fix. It prevents initialization MarkerClusterer when window is not defined (on server side rendering). So marker clusterer works correctly in both cases now.

It's published in npm. https://www.npmjs.com/package/js-marker-clusterer-universal

js-marker-clusterer dependency should be replaced by js-marker-clusterer-universal to take effect.

Thanks a lot man, you just saved me... kudos :)

alexnoise79 · 2019-10-19T12:23:30Z

i still have this issue, why is it marked as closed??

lazarljubenovic · 2019-10-19T13:59:14Z

You literally downvoted the explanation on why it was closed. You've seen why it was closed.

ghost · 2019-10-19T17:26:47Z

@lazarljubenovic we are going to move to https://github.com/googlemaps/v3-utility-library/tree/master/packages/markerclustererplus anyway

alexnoise79 · 2019-10-19T18:27:42Z

You literally downvoted the explanation on why it was closed. You've seen why it was closed.

so now we can reopen it 💃

anyway is not simpler to import js-marker-clusterer inside the library and create an exportable module?

i did this on mine:

https://github.com/alexnoise79/js-marker-clusterer (see src/markerclusterer.js)

and then on:
node_modules/@agm/js-marker-clusterer/fesm2015/agm-js-marker-clusterer.js

i replaced declare Markerclusterer with:

import * as MarkerClusterer from 'js-marker-clusterer';

it works and compile ssr 100%

IgorKurkov · 2020-04-13T08:09:44Z

My solution base on @mbrezovsky comment above - so

install his npm package https://www.npmjs.com/package/js-marker-clusterer-universal (or make own fork...)
make few commands in package.json

"install:clusterer-universal": "npm i js-marker-clusterer-universal && rm -rf node_modules/js-marker-clusterer && mv node_modules/js-marker-clusterer-universal node_modules/js-marker-clusterer",
"postbuild": "npm run install:clusterer-universal"

and it will be installed after all and replaced.
And SSR works! :)

jigfox self-assigned this Dec 4, 2017

mbrezovsky mentioned this issue Jan 8, 2018

fix(broken-clusters-with-angular-universal): fix error of js-marker-c… #1281

Closed

lazarljubenovic mentioned this issue Feb 26, 2018

Please, consider replacing original 'js-marker-clusterer' with up to date fork 'gmaps-marker-clusterer' #1284

Closed

stale bot added the stale label Nov 13, 2018

stale bot closed this as completed Nov 20, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clusters are breaking the universal app #1241

Clusters are breaking the universal app #1241

lazarljubenovic commented Nov 22, 2017

sebholstein commented Dec 2, 2017

jigfox commented Dec 4, 2017

jigfox commented Dec 17, 2017

lazarljubenovic commented Dec 17, 2017 •

edited

jigfox commented Dec 19, 2017 •

edited

lazarljubenovic commented Dec 19, 2017

lazarljubenovic commented Dec 20, 2017

lazarljubenovic commented Dec 22, 2017

cmddavid commented Dec 27, 2017

lazarljubenovic commented Dec 27, 2017 via email

cmddavid commented Dec 28, 2017

lazarljubenovic commented Dec 28, 2017 via email

mbrezovsky commented Jan 3, 2018 •

edited

cmddavid commented Jan 7, 2018 •

edited

jonaspraem commented Apr 17, 2018

cmddavid commented Apr 17, 2018

jonaspraem commented Apr 17, 2018

cmddavid commented Apr 17, 2018 •

edited

jonaspraem commented Apr 17, 2018

maksimbykov commented May 30, 2018

cmddavid commented May 30, 2018 •

edited

maksimbykov commented May 31, 2018

stale bot commented Nov 13, 2018

newprogrammer1991 commented Feb 12, 2019

nrozic commented Jul 8, 2019

alexnoise79 commented Oct 19, 2019

lazarljubenovic commented Oct 19, 2019

ghost commented Oct 19, 2019

alexnoise79 commented Oct 19, 2019 •

edited

IgorKurkov commented Apr 13, 2020 •

edited

Clusters are breaking the universal app #1241

Clusters are breaking the universal app #1241

Comments

lazarljubenovic commented Nov 22, 2017

sebholstein commented Dec 2, 2017

jigfox commented Dec 4, 2017

jigfox commented Dec 17, 2017

lazarljubenovic commented Dec 17, 2017 • edited

jigfox commented Dec 19, 2017 • edited

lazarljubenovic commented Dec 19, 2017

lazarljubenovic commented Dec 20, 2017

lazarljubenovic commented Dec 22, 2017

cmddavid commented Dec 27, 2017

lazarljubenovic commented Dec 27, 2017 via email

cmddavid commented Dec 28, 2017

lazarljubenovic commented Dec 28, 2017 via email

mbrezovsky commented Jan 3, 2018 • edited

cmddavid commented Jan 7, 2018 • edited

jonaspraem commented Apr 17, 2018

cmddavid commented Apr 17, 2018

jonaspraem commented Apr 17, 2018

cmddavid commented Apr 17, 2018 • edited

jonaspraem commented Apr 17, 2018

maksimbykov commented May 30, 2018

cmddavid commented May 30, 2018 • edited

maksimbykov commented May 31, 2018

stale bot commented Nov 13, 2018

newprogrammer1991 commented Feb 12, 2019

nrozic commented Jul 8, 2019

alexnoise79 commented Oct 19, 2019

lazarljubenovic commented Oct 19, 2019

ghost commented Oct 19, 2019

alexnoise79 commented Oct 19, 2019 • edited

IgorKurkov commented Apr 13, 2020 • edited

lazarljubenovic commented Dec 17, 2017 •

edited

jigfox commented Dec 19, 2017 •

edited

mbrezovsky commented Jan 3, 2018 •

edited

cmddavid commented Jan 7, 2018 •

edited

cmddavid commented Apr 17, 2018 •

edited

cmddavid commented May 30, 2018 •

edited

alexnoise79 commented Oct 19, 2019 •

edited

IgorKurkov commented Apr 13, 2020 •

edited