vuestorefront / storefront-ui

A frontend library for Vue and React that helps developers quickly build fast, accessible, and beautiful storefronts. Made with 💚 by Vue Storefront team and contributors.
https://storefrontui.io
MIT License
2.33k stars 455 forks source link

[BUG]: window is not defined due to proactive leaflet loading #2499

Closed JesseMaxwell closed 1 year ago

JesseMaxwell commented 1 year ago

Describe the bug Page crashes because of zealous loading of 3rd-party module leaflet

How to reproduce Steps to reproduce the behavior:

  1. Install 0.13.5
  2. Enable SSR
  3. Load a Page
  4. See error

Expected behavior A clear and concise description of what you expected to happen.

Actual behavior What actual happened.

Code examples With Server Side Rendering enabled, Line 230 in node_modules/leaflet/dist/leaflet-src.js is hit causing the renderer to crash with the error:

ReferenceError
window is not defined
image

Running with Client Side rendering enabled shows the call stack in the browser:

image

This is my yarn.lock

image

Devices (please complete the following information):

┆Issue is synchronized with this Jira Task by Unito ┆Link To Issue: https://vsf.atlassian.net/browse/SFUI-304

JesseMaxwell commented 1 year ago

Side note: there is a discrepancy with versions:

JesseMaxwell commented 1 year ago

Downgrading to 0.13.3 "fixes" this problem.

bensinca commented 1 year ago

As downgrading is not a real 'fix' but thank you @JesseMaxwell. I think on files such as https://github.com/vuestorefront/storefront-ui/blob/develop/packages/vue/src/components/organisms/SfStoreLocator/SfStoreLocator.vue#L73

This bit should be on client side. Therefore, could a use of 'require' instead be the solution?

import { Icon } from "leaflet"; delete Icon.Default.prototype._getIconUrl; Icon.Default.mergeOptions({ iconRetinaUrl: require("leaflet/dist/images/marker-icon-2x.png"), iconUrl: require("leaflet/dist/images/marker-icon.png"), shadowUrl: require("leaflet/dist/images/marker-shadow.png"), });

sheronov commented 1 year ago

I also faced with this problem at 0.13.4 and 0.13.5 versions, because of the import from raw "leaflet".

I even don't use SfStoreLocator component at all. But it breaks every pages where I have at least one import { SfSomeComponent } from "@storefront-ui/vue"

Maybe something wrong with my SSR settings, but now I couldn't just update storefront-ui. If shouldn't be in this way with patch updates.

This PR #2428 brought this issue.

bensinca commented 1 year ago

Potential solutions: https://stackoverflow.com/questions/67824862/how-to-make-a-dynamic-import-in-nuxt/67825061#67825061

after @sheronov's comment I took out the process.client check.

async mounted() {  // or use useMounted hook
    const { icon } = await import('leaflet')
    delete Icon.Default.prototype._getIconUrl;
    Icon.Default.mergeOptions({
      iconRetinaUrl: require("leaflet/dist/images/marker-icon-2x.png"),
      iconUrl: require("leaflet/dist/images/marker-icon.png"),
      shadowUrl: require("leaflet/dist/images/marker-shadow.png"),
    });
},

Not tested

sheronov commented 1 year ago

@bensinca If everything will be in mounted() part then if (process.client) is redundant.

As I know the mounted block doesn't compile in SSR. But I do not use NUXT, just an old vue-storefront-1)

So in my case the process.client will generate another frontend error in clients browsers.

bensinca commented 1 year ago

thanks @sheronov for pointing that out.

I tested this https://github.com/vuestorefront/storefront-ui/pull/2501 on my dev env it seems to fix the window is not defined error - but I didn't test to see if the component is working :see_no_evil:

FRSgit commented 1 year ago

Hey everyone!

I'm closing this issue as it seems that #2501 by @bensinca has fixed it. Please open a new ticket if you still can reproduce the problem.

I wanted to inform you that SFUI1 was moved to LTS mode and we encourage you all to try out the new version - SFUI2!

Also, don't hesitate to contact us on our discord server if you have any questions!