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

On mount, carousel raises an error Cannot read properties of undefined (reading 'getIndex') when circular option is set to true. #616

Closed
kazvaggos opened this issue Feb 4, 2022 · 5 comments
Assignees
Labels
🔖3.x v3.x~ related issue 📦Vue2 @egjs/vue-flicking related issue

Comments

@kazvaggos
Copy link

kazvaggos commented Feb 4, 2022

Description

"@egjs/vue-flicking": "3.8.0"
On mount, carousel raises an error Cannot read properties of undefined (reading 'getIndex') when circular option is set to true.

TypeError: Cannot read properties of undefined (reading 'getIndex')
  at clonePanels(./node_modules/@egjs/flicking/dist/flicking.esm.js:3416:47)
  at updateClonePanels(./node_modules/@egjs/flicking/dist/flicking.esm.js:3202:12)
  at resize(./node_modules/@egjs/flicking/dist/flicking.esm.js:2401:10)
  at build(./node_modules/@egjs/flicking/dist/flicking.esm.js:3223:10)
  at new t(./node_modules/@egjs/flicking/dist/flicking.esm.js:2271:10)
  at new e(./node_modules/@egjs/flicking/dist/flicking.esm.js:4335:22)
  at call(./node_modules/@egjs/vue-flicking/dist/flicking.esm.js:125:29)
  at invokeWithErrorHandling(./node_modules/vue/dist/vue.esm.js:1872:57)
  at callHook(./node_modules/vue/dist/vue.esm.js:4244:7)
  at insert(./node_modules/vue/dist/vue.esm.js:3167:7)
  at invokeInsertHook(./node_modules/vue/dist/vue.esm.js:6401:28)
  at __patch__(./node_modules/vue/dist/vue.esm.js:6620:5)
  at _update(./node_modules/vue/dist/vue.esm.js:3972:19)
  at call(./node_modules/vue/dist/vue.esm.js:4090:10)
  at get(./node_modules/vue/dist/vue.esm.js:4504:25)
  at run(./node_modules/vue/dist/vue.esm.js:4579:22)
  at flushSchedulerQueue(./node_modules/vue/dist/vue.esm.js:4335:13)
  at i(./node_modules/vue/dist/vue.esm.js:1998:12)
  at MutationObserver.te(./node_modules/vue/dist/vue.esm.js:1924:12)

It doesn't happen all the time, but we noticed this error happening a lot on our error monitor tool.

Steps to check or reproduce

Unfortunately we cannot reproduce it locally. But we know that it is in this line:

var needCloneOnPrev = panelAtLeftBoundary.getIndex() !== 0 && panelAtLeftBoundary.getIndex() <= panelAtRightBoundary.getIndex();

panelAtLeftBoundary and panelAtRightBoundary start as undefined and one of them (or both) is not set in the for loops above this line.
Is there a reason why this is happening? Is it possible to add an extra check if panelAtLeftBoundary and panelAtRightBoundary exist in this line so it doesn't throw?

@WoodNeck
Copy link
Member

WoodNeck commented Feb 7, 2022

Hello @kazvaggos!
Can you provide me with some more information about this?
I've just tested it with the same version(3.8.0), but couldn't reproduce that error.

@WoodNeck WoodNeck self-assigned this Feb 7, 2022
@WoodNeck WoodNeck added 🔖3.x v3.x~ related issue 📦Vue2 @egjs/vue-flicking related issue labels Feb 7, 2022
@kazvaggos
Copy link
Author

Hello @WoodNeck,
Unfortunately we can't reproduce it either. But our error monitor tool reports it at least 10 times per hour, meaning that it happens quite often.
The only thing I can guess, is that the content of the carousel or the parent that contains the carousel is not fully loaded on mount, and for some reason when the carousel builds with the option circular: true, it tries to resize and then clone panels. The panel cloning fails because of the error above.

Looking at the source code, the following lines break:
https://github.com/naver/egjs-flicking/blob/v3/src/components/Viewport.ts#L1265-L1303

The reason for this is because:

let panelAtLeftBoundary!: Panel;             // undefined
    for (const panel of reversedPanels) {
      if (!panel) {
        continue;
      }
      sizeSum += panel.getSize() + gap;
      if (sizeSum >= areaPrev) {              // always false
        panelAtLeftBoundary = panel;      // never accessed
        break;
      }
    }
    
    // or
    
    let panelAtRightBoundary!: Panel;       // undefined
    for (const panel of panels) {
      if (!panel) {
        continue;
      }
      sizeSum += panel.getSize() + gap;
      if (sizeSum >= areaNext) {              // always false
        panelAtRightBoundary = panel;    // never accessed
        break;
      }
    }
    
    // resulting:
   // Throws  'Cannot read properties of undefined (reading 'getIndex')'
   const needCloneOnPrev = panelAtLeftBoundary.getIndex() !== 0                
      && panelAtLeftBoundary.getIndex() <= panelAtRightBoundary.getIndex();

Probably it can be reproduced if the carousel tries to mount on a div that is with style display: none? Not very sure about that.
However, what I am asking is if could guard panelAtLeftBoundary and panelAtRightBoundary or the condition on the variable needCloneOnPrev with a fallback so it doesn't break. One easy solution that I could think is the following change:

   const needCloneOnPrev = panelAtLeftBoundary?.getIndex() !== 0                
      && panelAtLeftBoundary?.getIndex() <= panelAtRightBoundary?.getIndex();

@WoodNeck
Copy link
Member

WoodNeck commented Feb 8, 2022

Probably it can be reproduced if the carousel tries to mount on a div that is with style display: none? Not very sure about that.

Certainly, it could be a possible cause for this issue. As there were a few issues related to panel size being 0 in the v3.
#619 fixes where you've suggested, I'll release it after it's merged.
Feel free to report any further issues after that :)

@WoodNeck
Copy link
Member

@kazvaggos
Released vue-flicking@3.8.1 that fixes this issue.
You can install it with

npm i @egjs/vue-flicking@latest-3

Please try that version, and reopen this issue if the bug still remains :)

@kazvaggos
Copy link
Author

@WoodNeck
Thanks a lot for the fix!
However, this fix introduced a much bigger bug with the above scenario described in #624

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔖3.x v3.x~ related issue 📦Vue2 @egjs/vue-flicking related issue
Projects
None yet
Development

No branches or pull requests

2 participants