Skip to content

test: migrate preact/utils to vitest #715

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 3 commits into from
Jul 2, 2025

Conversation

43081j
Copy link

@43081j 43081j commented Jul 1, 2025

This also adds preact/utils to the pnpm workspace so it can be
resolved properly via @preact/signals-utils.

Copy link

changeset-bot bot commented Jul 1, 2025

⚠️ No Changeset found

Latest commit: a47b329

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

This also adds `preact/utils` to the pnpm workspace so it can be
resolved properly via `@preact/signals-utils`.
@43081j 43081j force-pushed the vitest-preact-utils branch from fa11429 to bcab425 Compare July 1, 2025 08:34
@@ -1,6 +1,7 @@
packages:
# all packages in direct subdirs of packages/
- "packages/*"
- "packages/preact/utils"
Copy link
Author

Choose a reason for hiding this comment

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

we didn't actually need this in the end i think, because we're misusing sub-packages

we define preact/utils as its own package, @preact/signals-utils
but within this repo, we import it as if its a sub-path, @preact/signals/utils.

we don't actually publish this package afaict so this worked fine. but i wonder if we should update it to treat it as an actual package, i.e. change the imports and tsconfig refs to @preact/signals-utils

Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer not to, we use this pattern in the main preact repo as well

Copy link
Author

Choose a reason for hiding this comment

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

i will remove this change

it seems like signals/utils having a package.json doesn't make much sense, though. it should probably just be a subdir of signals. we don't expose it to pnpm, so its dependencies don't get install afaik, and nothing resolves to it as everything accesses it by subdir instead.

one for another time though

@43081j 43081j force-pushed the vitest-preact-utils branch from 7e86e6b to 828fd83 Compare July 1, 2025 18:11
@43081j
Copy link
Author

43081j commented Jul 1, 2025

ok it turns out we do need the pnpm-workspace change @JoviDeCroock

in order for the tests in the utils package to resolve bare specifiers, the utils package itself needs to be picked up by pnpm. so it has to be in the workspace file. but i haven't touched the imports, etc.

i've also downgraded vite in this PR which should fix the failing CI (vite 7 doesn't work in node 18)

we should merge this one before the other PRs

@JoviDeCroock JoviDeCroock merged commit 02bde88 into preactjs:vitest-migration Jul 2, 2025
1 check passed
@43081j 43081j deleted the vitest-preact-utils branch July 2, 2025 18:25
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