Drop globby as a dependency

[PodaruDragos]

Hello, as v6 is just around the block please consider dropping globby as a dependency.
Just recursively searching for files in a dir can be obtained without 23 dependencies.

Example code ( tested locally on random code, not Mikro )

export function glob(
  pattern: RegExp,
  basePath: string
): string[] {
  const files: string[] = [];

  function traverseDirectory(directory: string): void {
    const entries = readdirSync(directory);

    for (const entry of entries) {
      const entryPath = join(directory, entry);
      const stat = statSync(entryPath);

      if (stat.isDirectory()) {
        traverseDirectory(entryPath);
      } else if (stat.isFile() && entry.match(pattern)) {
        files.push(entryPath);
      }
    }
  }

  traverseDirectory(basePath);
  return files;
}



const __filename = fileURLToPath(import.meta.url);
const __dirname = dirname(__filename);

const files = glob(/\.ts$/, join(__dirname, './entities'));
console.log({ files });

What do you think ?

[B4nan]

I can't say I like the dependency, but I have so many real problems to solve, that I am simply not willing to spend time on making a globby replacement myself. Feel free to PR that if you have the time and you consider this important for you - I am all in for a smaller dependency footprint. But I want something that is pretty much a drop-in replacement (I don't care about the API, feature-wise). We need globs, not regexps. We also need negative globs. And such a thing should be well tested on windows too (I can't do that myself and not really interested in slow CI windows tests either, the CI is already very slow with linux).

But that is pretty much it, it's not that unreal to achieve this. It's just about the time and priorities, and I have way too much more important stuff right now.

Worth saying that if we make it a compatible replacement, it can happen any time, as it won't be breaking. That is what I'd envision ideally.

[PodaruDragos]

Fair enough, by looking at the code I can see globby being used in 3 places.
FileCacheAdapter, MetadataDiscovery and SeedManager. now there are no negating paths no where there, it's just getting files from a directory as far as I can tell. Is there any reason why you would want to support other things ?

I can test this on windows, since I am working from a windows machine.
In regards of globs, I am sure I can adapt that function to work with something like : '/**/entities/*.ts' and '!/**/entities/*.ts'.

I'll try to cook up something and hopefully if I can run the tests on my local machine I will make a PR.

[B4nan]

now there are no negating paths no where there

This is user-facing, it's not about internal usage.

[B4nan]

Btw maybe we could use some lower-level dependency of globby to make things a bit simpler.

And one more note, globs are generally more powerful than just handling **, e.g. there are things like file-{a,b,c}.jpg supported. We don't necessarily need to support those out of box, but if we won't, we should have an easy way to opt into the previous behavior (or bring the globby back in from outside).

[PodaruDragos]

well, I have tried a quick prototype. Writing a function with no dependencies that can also handle globs like this !(*.d).{js,ts} is complicated to say the least. I will spend some more time on this, but I think this is probably more complicated than I initially thought.

[B4nan]

Yeah but maybe we could just pick some of the lower level dependency (e.g. picomatch).

[PodaruDragos]

Hey Martin, good news

glob pr.

I mean it's still not there and probably @types/node won't have it for a long time but this is really good.

PS: it's using minimatch so it should support all usecases

[PodaruDragos]

to be fair things are looking so good in all aspects, when v8 ships native decorators and decorators metadata and it's chery picked into node.js, reflect-metadata can go as well.

If I am not mistaken, mikro does not use class parameter decorators right ?

[B4nan]

If I am not mistaken, mikro does not use class parameter decorators right ?

nope, but maybe we should :D now to get the parameter names we need to parse the JS AST, which also brings one fat dependency on acorn (that slows down the discovery significantly, IIRC its literally the only real bottleneck). in the early versions, i was using regexps to parse things manually, as i didn't want to bring fat deps for such a small reason, but it was so broken that i moved to escaya and later on to acorn as escaya wasn't maintained (at all)

[PodaruDragos]

if you only want to get lets say we have this

@Entity()
export class SomeEntity {
  @Property()
  public property
}

you can read property with the new class based decorators, I am talking about vanilla decorators. I can post an example if you are interested. Ofc that would be a massive breaking change haha, but it is possible

[B4nan]

I am talking about constructor (and toJSON) parameters, the ORM needs to know their names when it invokes them. And we parse the AST to get those names currently.

https://github.com/mikro-orm/mikro-orm/blob/master/packages/core/src/utils/Utils.ts#L474

(we actually use esprima nowadays, used to be acorn and escaya before)

[PodaruDragos]

toJSON can be read if it's decorated, with constructor is not possible if they don't advance the parameter decorators.