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

chore(deps): update tinyglobby and use isDynamicPattern #6600

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

hi-ogawa
Copy link
Contributor

@hi-ogawa hi-ogawa commented Oct 1, 2024

Description

Replaced isDynamicPattern with the one from tinyglobby. It also fixes a symlink issue reported by storybook #6274 (comment)
Thanks @SuperchupuDev for the new release!

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 Oct 1, 2024

Deploy Preview for vitest-dev ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit f40d32e
🔍 Latest deploy log https://app.netlify.com/sites/vitest-dev/deploys/66fbc6190b095d0008161a0e
😎 Deploy Preview https://deploy-preview-6600--vitest-dev.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@SuperchupuDev
Copy link
Contributor

FYI I've just released a 0.2.8 hotfix 😅 you might want to update to that instead

@hi-ogawa hi-ogawa changed the title chore(deps): update tinyglobby v0.2.7 and use isDynamicPattern chore(deps): update tinyglobby and use isDynamicPattern Oct 1, 2024
@hi-ogawa
Copy link
Contributor Author

hi-ogawa commented Oct 1, 2024

@SuperchupuDev It looks like this exotic case #6274 (comment) came back as failing/timeout? I ran it locally on my Linux PC and this indeed gets stack.

pnpm -C test/reporters test custom-reporter -- -t 'load no base'

@SuperchupuDev
Copy link
Contributor

SuperchupuDev commented Oct 1, 2024

interesting, i'll take a look tomorrow. meanwhile, can you try to tell me the exact glob usage that makes it get stuck? basically a minimal repro

@hi-ogawa
Copy link
Contributor Author

hi-ogawa commented Oct 1, 2024

@SuperchupuDev I tried a repro but it looks quite puzzling. It's likely that this is related to the scenario like SuperchupuDev/tinyglobby#53, but behavior-wise, it's same as v0.2.6, so maybe something happened internally which is affecting performance of large node_modules crawling.

I can only reproduce the difference between v0.2.6 and v0.2.8 on Vitest repo's node_modules. I added test/reporters/repro.mjs, which should stuck only on v0.2.8. I hope it would help the investigation.


CI is passing for Mac and Windows, but this is because the test is skipped on CI, so I would assume the issue is same for non Linux too:

describe('custom reporters', () => {
// On Windows and macOS child_process is very unstable, we skip testing it as the functionality is tested on Linux
if ((process.platform === 'win32' || process.platform === 'darwin') && process.env.CI) {
return test.skip('skip on windows')
}

@SuperchupuDev
Copy link
Contributor

i think i know what's going on, node_modules are linked to each other when using pnpm, making fdir recursively crawl those. if two packages use each other it probably enters an infinite loop. i wonder how fast-glob handles that

@SuperchupuDev
Copy link
Contributor

@hi-ogawa can you tell me if it works using "tinyglobby": "https://pkg.pr.new/tinyglobby@1f1fa12" in the package.json? let me know if it does then i'll do a new release

@hi-ogawa
Copy link
Contributor Author

hi-ogawa commented Oct 1, 2024

@SuperchupuDev Thanks for quick follow up. I confirmed https://pkg.pr.new/tinyglobby@1f1fa12 is working locally. I'll push it here to check CI now.

@hi-ogawa
Copy link
Contributor Author

hi-ogawa commented Oct 1, 2024

i think i know what's going on, node_modules are linked to each other when using pnpm, making fdir recursively crawl those. if two packages use each other it probably enters an infinite loop. i wonder how fast-glob handles that

Good spot. That might be it since we have a cyclic workspace warning by pnpm like this:

$ pnpm i
Scope: all 47 workspace projects
 WARN  There are cyclic workspace dependencies: /home/hiroshi/code/others/vitest/packages/browser, /home/hiroshi/code/others/vitest/packages/vitest
...

@SuperchupuDev
Copy link
Contributor

great, glad that it works. i've just released 0.2.9 that reverts the symlinks resolution support

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