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(browser): allow for pretty-format as a sibling dependency #4590

Merged
merged 1 commit into from
Nov 27, 2023

Conversation

nicojs
Copy link
Contributor

@nicojs nicojs commented Nov 25, 2023

Description

Solves #4570 by adding a missing dependency of @vitest/snapshot to the dependency optimization.

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

  • It's really useful if your PR references an issue where it is discussed ahead of time. If the feature is substantial or introduces breaking changes without a discussion, PR might be closed.
  • Ideally, include a test that fails without this PR but passes with it.
  • Please, don't make changes to pnpm-lock.yaml unless you introduce a new test example.

Tests

  • Run the tests with pnpm test:ci.

Documentation

  • If you introduce new functionality, document it. You can run documentation with pnpm run docs command.

Changesets

  • Changes in changelog are generated from PR name. Please, make sure that it explains your changes in an understandable manner. Please, prefix changeset messages with feat:, fix:, perf:, docs:, or chore:.

Copy link

netlify bot commented Nov 25, 2023

Deploy Preview for fastidious-cascaron-4ded94 canceled.

Name Link
🔨 Latest commit 8ac8628
🔍 Latest deploy log https://app.netlify.com/sites/fastidious-cascaron-4ded94/deploys/65623bde10faaa00084079a9

@nicojs
Copy link
Contributor Author

nicojs commented Nov 25, 2023

  • I've got the feeling 'vitest > pretty-format', can be removed from dependency optimization since vitest doesn't depend on pretty-format (anymore?). Let me know if this is correct, and I'll remove it.
  • Do you want me to add a test for this? If so, please point me in the right direction. For example, can I add pretty-format as a dependency to "test/browser"?

@nicojs nicojs changed the title fix(browser): add missing dep opimization fix(browser): allow for pretty-format as a sibling dependency Nov 25, 2023
@AriPerkkio AriPerkkio linked an issue Nov 26, 2023 that may be closed by this pull request
6 tasks
Copy link
Member

@AriPerkkio AriPerkkio left a comment

Choose a reason for hiding this comment

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

Tested this manually by patching node_modules using the linked minimal reproduction.

It also looks like this issue doesn't come up with pnpm. The vitest-tests repository has a browser setup with pnpm only: https://github.com/vitest-tests/browser-simple. As Vitest's main repository (this project) is using pnpm it's difficult to create failing test cases here. Maybe we'll need vitest-tests/browser-simple-npm.

@ghiscoding
Copy link
Contributor

ghiscoding commented Nov 27, 2023

Tested this manually by patching node_modules using the linked minimal reproduction.

It also looks like this issue doesn't come up with pnpm. The vitest-tests repository has a browser setup with pnpm only: https://github.com/vitest-tests/browser-simple. As Vitest's main repository (this project) is using pnpm it's difficult to create failing test cases here. Maybe we'll need vitest-tests/browser-simple-npm.

I have no idea if that would work or not, but have you ever thought of maybe using corepack for some tests or a workflow? I never personally use corepack myself, but I was wondering if it could help here or not? Maybe this GitHub Action setup-node issue 531 is related to what I just said which I think is not yet possible but they have a promising PR to support it but it seems extremely long to merge the PR

@sheremet-va sheremet-va merged commit ed50a94 into vitest-dev:main Nov 27, 2023
15 of 16 checks passed
@patrickberkeley
Copy link

@sheremet-va sorry to bother you, but any chance you could cut another beta release for this fix?

@nicojs nicojs deleted the fix/optimize-deps-snapshot branch November 27, 2023 19:37
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.

Vitest v1.0.0-beta5 browser mode hangs in an infinite reload
5 participants