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

fix: leafleat module import error in ssr mode #474

Merged
merged 4 commits into from
Feb 9, 2024

Conversation

rockleona
Copy link
Contributor

Types of changes

  • Bugfix

Description

  1. Install nuxt-leaflet
  2. Remove plugins/leaflet.js in nuxt.config.js
  3. Remove every imported vue2-leaflet components in components/venue/VenueMap.vue
  4. Remove icon related import
  5. Confirmed Step 4 is the problem that cause window is not defined error

Steps to Test This Pull Request

using node v16.x

Steps to reproduce the behavior:

  1. npm install --legacy-peer-deps
  2. npm run dev
  3. Go to browser and enter url http://localhost:3000/2024/zh-hant/venue

Related Issue

resolve #471

Copy link

netlify bot commented Feb 5, 2024

👷 Deploy request for ephemeral-sable-89c7e0 pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit b93c2c3

@rockleona rockleona requested review from josix and SivanYeh and removed request for josix February 5, 2024 17:02
@josix josix requested a review from baby230211 February 6, 2024 02:31
@SivanYeh
Copy link
Collaborator

SivanYeh commented Feb 6, 2024

It worked in "http://localhost:3000/2024/zh-hant/venue" on my laptop(with node v16.20.2):
image

However it didn't work on landing page:
image

Maybe 2024 landing image wasn't updated? Have you tried git rebase with main branch?

@rockleona
Copy link
Contributor Author

Maybe 2024 landing image wasn't updated? Have you tried git rebase with main branch?

May I ask which commit had updated the image? I'll check later to fix this.
But if 2024 landing image works well in the lastest commit, and do not have any conlict, I think it will be fine.

@SivanYeh
Copy link
Collaborator

SivanYeh commented Feb 6, 2024

Maybe 2024 landing image wasn't updated? Have you tried git rebase with main branch?

May I ask which commit had updated the image? I'll check later to fix this. But if 2024 landing image works well in the lastest commit, and do not have any conlict, I think it will be fine.

Here you are! Check these two:
3a35ccf

c63b93c
cc @mattwang44

@rockleona
Copy link
Contributor Author

rockleona commented Feb 6, 2024

Maybe 2024 landing image wasn't updated? Have you tried git rebase with main branch?

May I ask which commit had updated the image? I'll check later to fix this. But if 2024 landing image works well in the lastest commit, and do not have any conlict, I think it will be fine.

Here you are! Check these two: 3a35ccf

c63b93c cc @mattwang44

hmm....I've check that these commits are in this PR as well, that's weird,
I'll check it later and update with you.

@mattwang44
Copy link
Member

mattwang44 commented Feb 6, 2024

The index page and venue page work fine on my side.

rm -rf node_modules dist
nvm use lts/fermium  # v14.21.3
npm i
npm run dev

# run `npm run json-server` from another terminal

However, I got these messages from the terminal. Could be existing issues.

image

@rockleona rockleona force-pushed the fix/leaflet-ssr-window-not-defined branch from 65c2cbe to 20baac0 Compare February 7, 2024 03:09
@rockleona
Copy link
Contributor Author

The index page and venue page work fine on my side.

rm -rf node_modules dist
nvm use lts/fermium  # v14.21.3
npm i
npm run dev

# run `npm run json-server` from another terminal

However, I got these messages from the terminal. Could be existing issues.

image

Hi @SivanYeh, I've rebase the code and follow the steps that Matt tested,
it works fine to me, would you give it a try?

@@ -163,7 +163,7 @@ export default {
},
fetchOnServer: false,
computed: {
...mapState(['sponsorsData']),
// ...mapState(['sponsorsData']),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @mattwang44, I've comment out this line and L121, whiche can fix the error.

That error you'd mentioned is based on the setup for archive,
I think it will works the same when we use asyncData in the file

Comment on lines +85 to +104
mounted() {
this.getLeaflet()
},
methods: {
centerMap() {
this.$nextTick(() => {
this.$refs.leafletMap.mapObject.panTo([25.040997, 121.611417])
})
},
async getLeaflet() {
this.L = await import('leaflet')
this.icon = this.L.icon({
iconUrl: '/snake.png',
shadowUrl: '/snake-bg.png',
iconSize: [42, 42],
iconAnchor: [21, 21],
shadowSize: [45, 55],
shadowAnchor: [25, 30],
})
},
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks to @baby230211 suggestion, the code works well.

@SivanYeh
Copy link
Collaborator

SivanYeh commented Feb 7, 2024

It worked perfectly here too! You nailed it Leon. I opened two terminals and followed the steps outlined below:
Terminal1:

nvm use v16
npm run json-server

Terminal2:

nvm use v16
npm install --legacy-peer-deps
npm run dev
Go to browser and enter url http://localhost:3000/2024/zh-hant/venue

Interesting stuff, not vital at the moment but we may have to keep in mind:
Wen I tried to reproduce Matt’s instructions, I got error caused by v14:

ERROR: npm v10.4.0 is known not to run on Node.js v14.21.3.  This version of npm supports the following node versions: `^18.17.0 || >=20.5.0`. You can find the latest version at https://nodejs.org/.

ERROR:
/Users/cat/.nvm/versions/node/v14.21.3/lib/node_modules/npm/node_modules/@npmcli/agent/lib/agents.js:105
    options.lookup ??= this.#options.lookup
                   ^^^

SyntaxError: Unexpected token '??='
    at wrapSafe (internal/modules/cjs/loader.js:1029:16)
    at Module._compile (internal/modules/cjs/loader.js:1078:27)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1143:10)
    at Module.load (internal/modules/cjs/loader.js:979:32)
    at Function.Module._load (internal/modules/cjs/loader.js:819:12)
    at Module.require (internal/modules/cjs/loader.js:1003:19)
    at require (internal/modules/cjs/helpers.js:107:18)
    at Object.<anonymous> (/Users/cat/.nvm/versions/node/v14.21.3/lib/node_modules/npm/node_modules/@npmcli/agent/lib/index.js:7:15)
    at Module._compile (internal/modules/cjs/loader.js:1114:14)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1143:10)

These errors can be solved by using node v16.

Copy link
Collaborator

@SivanYeh SivanYeh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@baby230211
Copy link
Collaborator

Thanks @rockleona
This looks great, could you also help remove unused leaflet code?

  • plugins/leaflet.js
  • Nuxt.config.js

@rockleona
Copy link
Contributor Author

@baby230211 , I just removed them, have a look if youre free.

Copy link
Collaborator

@baby230211 baby230211 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@tyuchx
Copy link
Contributor

tyuchx commented Feb 8, 2024

LGTM

@SivanYeh SivanYeh merged commit bc9e170 into pycontw:main Feb 9, 2024
1 check passed
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.

[Bug Report] development environment setup failure in local
5 participants