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

refactor(useMount): no need to add optional when invoking fn #2590

Closed
wants to merge 1 commit into from

Conversation

FEliuyg
Copy link
Contributor

@FEliuyg FEliuyg commented Jul 9, 2024

🤔 这个变动的性质是?

  • 新特性提交
  • 日常 bug 修复
  • 站点、文档改进
  • 演示代码改进
  • TypeScript 定义更新
  • 包体积优化
  • 性能优化
  • 功能增强
  • 国际化改进
  • 重构
  • 代码风格优化
  • 测试用例
  • 分支合并
  • 其他改动(是关于什么的改动?)

🔗 相关 Issue

💡 需求背景和解决方案

useMount 在前置逻辑中已经判断了是否为函数,所以在调用的时候不需要再加可选链去调用函数。

📝 更新日志

☑️ 请求合并前的自查清单

  • 文档已补充或无须补充
  • 代码演示已提供或无须提供
  • TypeScript 定义已补充或无须补充
  • Changelog 已提供或无须提供

@crazylxr
Copy link
Collaborator

不能这么改哦,前置判断里只有在 dev 并且只给了一个警告,用户也是可以传非函数的,传过来 js 就报错了,页面就崩了

@crazylxr crazylxr closed this Jul 11, 2024
@FEliuyg
Copy link
Contributor Author

FEliuyg commented Jul 12, 2024

不能这么改哦,前置判断里只有在 dev 并且只给了一个警告,用户也是可以传非函数的,传过来 js 就报错了,页面就崩了

@crazylxr 我理解你说的,是为了更好地兼容可能的情况,但纠正一下,应该是用户可以不传,如果传的是非函数,比如传个数字,按照上面的逻辑同样是会报错的,因为可选链只是判断是否为undefined,如果不是,是会去执行的,然后就算加了 ? 也会报错。如果为了兼容它传的是非函数,还得用 isFunction 判断一下。

我理解既然开发环境做了判断,后面的可以默认传的就是函数了,除非有种情况,它写了这个函数,没有看效果,直接发布了,不然开发环境就报错了。

另外,我也是看了 useUnmount 的实现

const useUnmount = (fn: () => void) => {
  if (isDev) {
    if (!isFunction(fn)) {
      console.error(`useUnmount expected parameter is a function, got ${typeof fn}`);
    }
  }

  const fnRef = useLatest(fn);

  useEffect(
    () => () => {
      fnRef.current();
    },
    [],
  );
};

这里其实也没有加 ?

理论上两个风格应该要统一。

@crazylxr
Copy link
Collaborator

crazylxr commented Jul 16, 2024

开发环境的警告经常会被忽略。

那我建议可以给 useUnmount 加上可选链

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.

2 participants