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(tree): throw exception when onLoad callback does not add children #786

Merged
merged 7 commits into from
Aug 25, 2021

Conversation

bljessica
Copy link
Contributor

Closes #772

@vercel
Copy link

vercel bot commented Aug 5, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/tusimple/naive-ui/75BvyJzejwFMJLyzM7pBQGQgdoWw
✅ Preview: https://naive-ui-git-fork-bljessica-dev-tree-tusimple.vercel.app

@codecov
Copy link

codecov bot commented Aug 5, 2021

Codecov Report

Merging #786 (4c15777) into main (dd92889) will decrease coverage by 0.01%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #786      +/-   ##
==========================================
- Coverage   45.07%   45.06%   -0.02%     
==========================================
  Files         509      509              
  Lines       12464    12467       +3     
  Branches     3503     3504       +1     
==========================================
  Hits         5618     5618              
- Misses       5847     5850       +3     
  Partials      999      999              
Impacted Files Coverage Δ
src/tree/src/Tree.tsx 16.34% <0.00%> (-0.13%) ⬇️
src/tree/src/TreeNode.tsx 26.92% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dd92889...4c15777. Read the comment docs.

Copy link
Collaborator

@07akioni 07akioni left a comment

Choose a reason for hiding this comment

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

我简单过了一下这个 PR,感觉处理的太复杂了。

从根本原因上讲,是试图去展开一个没有孩子的节点。

所以我们应该避免这个事,所以我们应该检查一下这个节点有没有 isShallowLoaded 就行了。

当然,不能用同样的 tmNode,因为增加 children 会导致整个 props.data 改变,影响 treeMateRef,所以需要从 treeMateRef 上面用同样的 key 来拿到新的对应节点,然后检查是不是 isShallowLoaded

至于 catch 的问题或许我们可以不处理(只要这个是用户自己导致的),加载出错就应该爆炸,框架兜底的话感觉不太好。

@Volankey
Copy link
Collaborator

Volankey commented Aug 6, 2021

我简单过了一下这个 PR,感觉处理的太复杂了。

从根本原因上讲,是试图去展开一个没有孩子的节点。

所以我们应该避免这个事,所以我们应该检查一下这个节点有没有 isShallowLoaded 就行了。

当然,不能用同样的 tmNode,因为增加 children 会导致整个 props.data 改变,影响 treeMateRef,所以需要从 treeMateRef 上面用同样的 key 来拿到新的对应节点,然后检查是不是 isShallowLoaded

至于 catch 的问题或许我们可以不处理(只要这个是用户自己导致的),加载出错就应该爆炸,框架兜底的话感觉不太好。

但是得给用户一个关闭loading的能力吧,不然爆炸了就一直loading只能刷新页面解决了,有可能就是个网络超时,重试一次就好了

@07akioni
Copy link
Collaborator

07akioni commented Aug 7, 2021

关闭loading的能力吧,不然爆炸了就一直loading只能刷新页面解决了,有

resolved 的时候 loadingKey 肯定是要去掉的,但是是否 expand 最后还是要看 children 有没有。

问题在于 loading resolve 之后有没有孩子我们是不知道的,需要我们自己来检查。

去掉 loadingKey 和 expand 是两件事。

@bljessica
Copy link
Contributor Author

@07akioni

Comment on lines 537 to 542
const toExpandedNode = displayTreeMateRef
.value!.getFlattenedNodes([...mergedExpandedKeys, key])
.find((node) => (node as any).key === key)
if (!toExpandedNode?.rawNode.children) {
return
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

nodeToBeExpanded = displayTreeMateRef.getNode(key)

Comment on lines 64 to 78
const removeFromLoadingKeysRef = (tmNode: TmNode): void => {
NTree.loadingKeysRef.value.splice(
NTree.loadingKeysRef.value.findIndex((key) => key === tmNode.key),
1
)
NTree.handleSwitcherClick(tmNode)
})
}
void onLoad(tmNode.rawNode)
.then(() => {
removeFromLoadingKeysRef(tmNode)
NTree.handleSwitcherClick(tmNode)
})
.catch((switcherClickError) => {
console.error(switcherClickError)
removeFromLoadingKeysRef(tmNode)
})
Copy link
Collaborator

Choose a reason for hiding this comment

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

          void onLoad(tmNode.rawNode)
            .then(() => {
              NTree.handleSwitcherClick(tmNode)
            })
            .finally(() => {
              NTree.loadingKeysRef.value.splice(
                NTree.loadingKeysRef.value.findIndex((key) => key === tmNode.key),
                1
              )
            })

Copy link
Collaborator

Choose a reason for hiding this comment

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

代码尽量要简洁

src/tree/src/Tree.tsx Outdated Show resolved Hide resolved
src/tree/src/TreeNode.tsx Outdated Show resolved Hide resolved
@07akioni 07akioni merged commit ed73168 into tusen-ai:main Aug 25, 2021
rhengles pushed a commit to arijs/naive-ui that referenced this pull request Oct 20, 2021
…tusen-ai#786)

* fix(tree): throw exception when onLoad callback does not add children

* refactor(tree): not expand when node has no children

* refactor(tree): handle no-children expand error

* Update src/tree/src/Tree.tsx

* Update src/tree/src/TreeNode.tsx

Co-authored-by: 07akioni <[email protected]>
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.

When the onload callback does not add children, the tree component throws an exception
3 participants