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

feat(solid): add solid statesHook #479

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

theqianqiu
Copy link

相关 Issue / Related Issue

[Issue ID]

这个 PR 是什么类型?/ What type of PR is this?

  • 错误修复 (Bug Fix)
  • 新功能 (Feature)
  • 代码重构 (Refactor)
  • TypeScript 类型定义修改 (Typings)
  • 文档修改 (Docs)
  • 代码风格更新 (Code style update)
  • 其他,请描述 (Other, please describe):

这个 PR 做了什么?/ What does this PR do?

[简要描述所做更改 / Describe the changes briefly]

文档 / Docs

[此次 PR 的文档 / Docs for this PR]

测试 / Testing

[描述测试结果 / Describe the test results.]

Copy link

changeset-bot bot commented Jul 24, 2024

⚠️ No Changeset found

Latest commit: aeead7d

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@JOU-amjs JOU-amjs changed the title feat(stateshook-solid): add unfinished feature feat(solid): add solid statesHook Jul 26, 2024
@github-actions github-actions bot removed the shared label Jul 26, 2024
@MeetinaXD
Copy link
Contributor

感谢 PR!目前发现了以下问题:

  1. 过半的更改都是关于修复 'React' must be in scope when using JSX 这个问题,请考虑在其他 PR 解决,或修改 PR 描述;
  2. 出现了无意义提交,只影响代码风格而不产生实际更改的修改不应该提交;
  3. 修改了根 tsconfig.json,具体请看文件中的描述;
  4. 新增的测试没通过

稍后记得补充 changeset 😃

@@ -22,7 +22,7 @@
"importHelpers": true,
"removeComments": false,
"esModuleInterop": true,
"jsx": "react",
"jsx": "react-jsx",
Copy link
Contributor

Choose a reason for hiding this comment

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

这里修改的原因是什么?

如有需要,请在 client 下的 tsconfig 中修改,以免影响其他模块。

Copy link
Contributor

Choose a reason for hiding this comment

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

请不要提交无意义更改

@JOU-amjs JOU-amjs self-requested a review July 30, 2024 05:51
@JOU-amjs JOU-amjs marked this pull request as draft July 30, 2024 05:51
@JOU-amjs JOU-amjs marked this pull request as ready for review August 6, 2024 12:13
Copy link
Contributor

@JOU-amjs JOU-amjs left a comment

Choose a reason for hiding this comment

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

综合所述,可能还有些需要修改之处,本来想直接给修改的,但这么久跟下来solid单测集成进来确实存在一些困难,你也可以尝试修改,或者我后面再抽空修改下

import { getStateCache } from '@/hooks/core/implements/stateCache';
import { useRequest } from '@/index';
import { key } from '@alova/shared/function';
import { fireEvent, screen, waitFor } from '@testing-library/dom';
Copy link
Contributor

Choose a reason for hiding this comment

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

这里应该要用@solid/testing-library,https://github.com/solidjs/solid-testing-library

},

onMounted: callback => {
createEffect(callback);
Copy link
Contributor

Choose a reason for hiding this comment

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

createEffect可能会带副作用,即callback内如果有solid状态的话,当状态改变时此callback依然会执行,因此这不是一个纯粹的onMounted钩子


render(() => (<Page />) as unknown as JSX.Element);

waitFor(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

没有添加await时检测不到回调内部的期望值,当添加上await时此单测是报错的,下同

@JOU-amjs JOU-amjs changed the base branch from main to test/vitest-migration October 25, 2024 08:50
@JOU-amjs JOU-amjs changed the base branch from test/vitest-migration to main October 25, 2024 09:04
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.

3 participants