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

what is the equivalent of nodir? #51

Closed
ghiscoding opened this issue Sep 30, 2024 · 9 comments
Closed

what is the equivalent of nodir? #51

ghiscoding opened this issue Sep 30, 2024 · 9 comments
Labels
question Further information is requested

Comments

@ghiscoding
Copy link

ghiscoding commented Sep 30, 2024

Hello, I'm trying to migrate from globby to tinyglobby in Lerna-Lite, I was waiting for the missing option followSymbolicLinks before switching which you just reloeased today (great) but I'm getting 1 unit test failure that used the nodir with globby, it doesn't look like tinyglobby supports that option and I tried to use onlyDirectories: false instead but that doesn't help (I also tried nodir nonetheless and failed, and I also even tried to remove the option completely but again it also failed). Here's my Lerna-Lite PR to do the switch, and my unit test file.

Here's my previous code (in my Lerna-Lite make-diff-predicate.ts file)

// optionally exclude sub-packages
if (diffOpts?.independentSubpackages) {
  independentSubpackages = globbySync('**/*/package.json', {
    cwd: formattedLocation,
    nodir: true,
    ignore: ['**/node_modules/**'],
  }).map((file) => `:^${formattedLocation}/${dirname(file)}`);
}

and here's what I tried

+ import { globSync } from 'tinyglobby';

// optionally exclude sub-packages
if (diffOpts?.independentSubpackages) {
  independentSubpackages = globSync('**/*/package.json', {
    cwd: formattedLocation,
-   nodir: true,
+   onlyDirectories: false,
    ignore: ['**/node_modules/**'],
  }).map((file) => `:^${formattedLocation}/${dirname(file)}`);
}

You can see my unit test failure in this CI PR workflow

⎯⎯⎯⎯⎯⎯⎯ Failed Tests 1 ⎯⎯⎯⎯⎯⎯⎯

 FAIL  packages/core/src/utils/collect-updates/__tests__/lib-make-diff-predicate.spec.ts > exclude subpackages when --independent-subpackages option is enabled and nested package.json is found
AssertionError: expected last "execSync" call to have been called with [ 'git', …(2) ]

- Expected
+ Received

  Array [
    "git",
    Array [
      "diff",
      "--name-only",
      "v1.0.0",
      "--",
      "packages/pkg-2",
-     ":^packages/pkg-2/packages/pkg-2/and-another-thing",
    ],
    Object {
      "cwd": "/test",
    },
  ]

 ❯ packages/core/src/utils/collect-updates/__tests__/lib-make-diff-predicate.spec.ts:93:35
     91| 
     92|   expect(result).toBe(true);
     93|   expect(childProcesses.execSync).toHaveBeenLastCalledWith(
       |                                   ^
     94|     'git',
     95|     ['diff', '--name-only', 'v1.0.0', '--', 'packages/pkg-2', ':^packa…

It looks like it's the only failure that I have before being to switch to tinyglobby, any help would be super appreciate. Thanks :)

@SuperchupuDev SuperchupuDev added the question Further information is requested label Sep 30, 2024
@SuperchupuDev
Copy link
Owner

nodir doesn't seem to be a valid option in globby or fast-glob, but it is an option in glob. fast-glob states that the equivalent of nodir is onlyFiles which is enabled by default. can you try to remove onlyDirectories from the options?

@ghiscoding
Copy link
Author

ghiscoding commented Sep 30, 2024

nodir doesn't seem to be a valid option in globby or fast-glob, but it is an option in glob. fast-glob states that the equivalent of nodir is onlyFiles which is enabled by default.

it is an option as can be seen in this globby issue but it was undocumented, probably because it was an indirect option from glob which globby extends from.

can you try to remove onlyDirectories from the options?

I did try to remove it completely as mentioned above, but it still fails and it also fails with onlyFiles: true as well. The call for that globSync doesn't return anything as opposed to before

image

oh I also noticed another issue which is even worst, I have some code to cleanup temp folders via an npm script and is also an option Lerna-Lite itself and that also is failing, in fact it goes into an infinite loop 😨 But that was working fine with globby and I'm on Windows if it makes any difference.

import { removeSync } from 'fs-extra/esm';
import { join as pathJoin } from 'node:path';
import normalizePath from 'normalize-path';
import tempDir from 'temp-dir';
import { glob } from 'tinyglobby';

glob(normalizePath(pathJoin(tempDir, '/lerna-*')), { onlyDirectories: true })
  .then((deleteFolders) => {
    // silently delete all files/folders that startsWith "lerna-"
    console.log(`Found ${deleteFolders.length} temp folders to cleanup.`);
    (deleteFolders || []).forEach((folder) => removeSync(folder));
  })
  .catch((error) => {
    console.error('Error occurred while cleaning up temp folders:', error);
  });

it's supposed to delete folders like below but the script doesn't find anything, neither stops which is bad

image

I wonder if in both cases the pattern has to be changed or is invalid compare to previous globby pattern?

@SuperchupuDev
Copy link
Owner

SuperchupuDev commented Sep 30, 2024

I did try to remove it completely as mentioned above, but it still fails and it also fails with onlyFiles: true as well. The call for that globSync doesn't return anything as opposed to before

can you give me the exact pattern that fails?

oh I also noticed another issue which is even worst, I have some code to cleanup temp folders via an npm script and is also an option Lerna-Lite itself and that also is failing, in fact it goes into an infinite loop but that was working fine with globby. I'm on Windows if it makes any difference.

try using deep: 0 as an option. it probably tries to scan the whole temp directory which might be huge and as such takes way too long to execute, unfortunately it can't optimize it until #46 gets resolved

@ghiscoding
Copy link
Author

try using deep: 0 as an option. it probably tries to scan the whole temp directory which might be huge and as such takes way too long to execute, unfortunately it can't optimize it until #46 gets resolved

nope that doesn't seem to help and this is rather quick with globby (1-2sec) but infinite loop with tinyglobby. here's the console log of the normalize path which is provided as the glob pattern

C:/Users/User/AppData/Local/Temp/lerna-*

@SuperchupuDev
Copy link
Owner

try using deep: 0 as an option. it probably tries to scan the whole temp directory which might be huge and as such takes way too long to execute, unfortunately it can't optimize it until #46 gets resolved

nope that doesn't seem to help and this is rather quick with globby (1-2sec) but infinite loop with tinyglobby. here's the console log of the normalize path which is provided as the glob pattern

C:/Users/User/AppData/Local/Temp/lerna-*

can reproduce, seems like the root inferrer sets the root at C:/Users, which scans everything in every user folder 😰 will try to fix

@SuperchupuDev
Copy link
Owner

is the pattern you're using in the first comment (the one from lerna-lite) an absolute path? if so it might be caused by the same issue

@SuperchupuDev
Copy link
Owner

okay i see the issue in the temp directory thing. since the cwd isn't explicitly set, it defaults to process.cwd(). this makes the pattern get converted to ../../../<whatever path to temp>, and since common paths aren't currently inferred that way (see #22), it will scan C:/Users/user or similar, which is too huge to be done in a reasonable time. as a workaround @ghiscoding, try this: (see the options change)

import { removeSync } from 'fs-extra/esm';
import { join as pathJoin } from 'node:path';
import normalizePath from 'normalize-path';
import tempDir from 'temp-dir';
import { glob } from 'tinyglobby';

glob(normalizePath(pathJoin(tempDir, '/lerna-*')), { absolute: true, cwd: tempDir, onlyDirectories: true })
  .then((deleteFolders) => {
    // silently delete all files/folders that startsWith "lerna-"
    console.log(`Found ${deleteFolders.length} temp folders to cleanup.`);
    (deleteFolders || []).forEach((folder) => removeSync(folder));
  })
  .catch((error) => {
    console.error('Error occurred while cleaning up temp folders:', error);
  });

@ghiscoding
Copy link
Author

the code from the 1st question was created by another user, not the original author, and it was documented in this comment and I'm just trying to replace globby with tinyglobby, the original code had this globbySync('**/*/package.json'. However the user doesn't even use it at this point, so I'm not sure how much time we should spend on this.

okay i see the issue in the temp directory thing. since the cwd isn't explicitly set, it defaults to process.cwd(). this makes the pattern get converted to ../../../<whatever path to temp>, and since common paths aren't currently inferred that way (see #22), it will scan C:/Users/user or similar, which is too huge to be done in a reasonable time. as a workaround @ghiscoding, try this: (see the options change)

yes you're right for the 2nd one, that works :)

image

@ghiscoding
Copy link
Author

ghiscoding commented Sep 30, 2024

ahhh I found my issue for the 1st question, I had a mock on globby but didn't rename it well for tinyglobby, then if I rename it properly it now works.. So it looks like I'm all good to proceed with the switch :)

const { globMock } = vi.hoisted(() => ({ globMock: vi.fn() }));
- vi.mock('glob', async () => ({
+ vi.mock('tinyglobby', async () => ({
  ...(await vi.importActual<any>('tinyglobby')),
  globSync: globMock,
}));

Thanks a lot for the help and making the globbing world much faster and smaller 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants