Skip to content

fix(Picker): ensure that the confirm event params are up to date #13381

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

Merged
merged 5 commits into from
Mar 23, 2025

Conversation

refinist
Copy link
Contributor

@refinist refinist commented Mar 7, 2025

// wait nextTick to ensure the model value is update to date
// when confirm event is emitted

如注释描述,需要在 nextTick 后去获取最新的值,所以要放在 nextTick 的回调里获取最新。
这块需要手动测试,在配置了 min-date 后,滚动还没有停止时,就快速直接点击 confirm 按钮的场景。

#11878

@codecov-commenter
Copy link

codecov-commenter commented Mar 7, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.75%. Comparing base (ec5b45b) to head (61819af).
Report is 64 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #13381      +/-   ##
==========================================
+ Coverage   89.60%   89.75%   +0.15%     
==========================================
  Files         257      257              
  Lines        7013     7021       +8     
  Branches     1736     1738       +2     
==========================================
+ Hits         6284     6302      +18     
+ Misses        384      380       -4     
+ Partials      345      339       -6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@inottn
Copy link
Collaborator

inottn commented Mar 7, 2025

Could you please add a test for the case? Thanks.

@refinist
Copy link
Contributor Author

refinist commented Mar 8, 2025

Could you please add a test for the case? Thanks.

hello,你可以看下我的 github 仓库 https://github.com/front-refined/vant-picker-issue 描述、视频和运行起来调试,这个 bug 存在偶发性,感谢

@inottn
Copy link
Collaborator

inottn commented Mar 8, 2025

你好,我对于这个 bug 的修复没有疑义,只是为了避免之后该 bug 的回归,我们最好有一个对应的单元测试。

@refinist
Copy link
Contributor Author

refinist commented Mar 8, 2025

你好,我对于这个bug的修复没有疑义,只是为了避免之后该bug的回归,我们最好有一个对应的单元测试。

你好,感谢答复,我尝试补充了单元测试,你看 ok 吗

@refinist
Copy link
Contributor Author

@inottn 你好,帮忙 review 下

@mouse-007
Copy link

这里的confirm的返回值与事件confirm输出的值不一致

@inottn
Copy link
Collaborator

inottn commented Mar 15, 2025

你好,单测需要其能验证该 PR 的改动:即在改动前运行应失败,改动后应通过。
因为是 Picker 组件的问题,需要将单元测试添加到 Picker 组件的相关文件中,并直接使用 Picker 组件编写单测。这有助于确保基于 Picker 的组件功能都正常。

@inottn
Copy link
Collaborator

inottn commented Mar 15, 2025

这里的 confirm 的返回值与事件 confirm 输出的值不一致

是的,但是修复 confirm 方法的返回值很困难,这需要返回一个 Promise。我们可以先修复事件输出的值。

@refinist
Copy link
Contributor Author

你好,单测需要其能验证该 PR 的改动:即在改动前运行应失败,改动后应通过。 因为是 Picker 组件的问题,需要将单元测试添加到 Picker 组件的相关文件中,并直接使用 Picker 组件编写单测。这有助于确保基于 Picker 的组件功能都正常。

感谢指点,我这里已经修改了测试用例,并且按照你说的自测通过。麻烦帮忙再看看 @inottn

@refinist
Copy link
Contributor Author

这里的 confirm 的返回值与事件 confirm 输出的值不一致

是的,但是修复 confirm 方法的返回值很困难,这需要返回一个 Promise。我们可以先修复事件输出的值。

是的,我有考虑过这个问题,但是如果修改了,对于开发者会有 Breaking changes,因为只能是 Promise 的方式了。

对于这个方法,我有几点想法:

  1. 理论上来讲这个主动调用 confirm 的方法应该用得很少
  2. 这个方法的使用一般会发生在下一个 DOM 更新周期之后吧,一般不会有数据上的问题
  3. 我们可以补充文档让开发者在调用 confirm 方法之前先 await nextTick()
  4. 等大版本升级处理返回一个 Promise

Copy link
Collaborator

@inottn inottn left a comment

Choose a reason for hiding this comment

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

Thanks! 👍

@refinist
Copy link
Contributor Author

Thanks! 👍

👍

@chenjiahan chenjiahan changed the title fix(Picker): The get parameter method should be placed in the correct… fix(Picker): ensure that the confirm event params are up to date Mar 23, 2025
Copy link
Member

@chenjiahan chenjiahan left a comment

Choose a reason for hiding this comment

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

Thank you!

@chenjiahan chenjiahan merged commit 3dc20ad into youzan:main Mar 23, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants