nextapps-de / flexsearch

Next-Generation full text search library for Browser and Node.js
Apache License 2.0
12.53k stars 491 forks source link

modernize codebase #343

Open ignatiusmb opened 2 years ago

ignatiusmb commented 2 years ago

Hey, thanks for the project! I saw there's some request for help with providing types at https://github.com/nextapps-de/flexsearch/discussions/339 but also a lot of things missing in the repo itself, I see there's a lot of things we can possibly improve here, one of them is of course improvements in DX and build tools.

Just a heads up, this PR looks like it contains lots of files with huge changes, I'll try my best to make them manageable in each of the commits, please let me know if you'd like me to separate them in multiple PRs instead, thanks again!


This PR is mainly an attempt to close some of the TS stuff like #243 and #342, I'll try and explain through each of the changes in the order they're commited

Planned TODOs

Marking this as a draft for now, @ts-thomas does this seem okay for me to continue on?

dr-charnog commented 2 years ago

@ignatiusmb you are doing a valuable contribution, thanks for that and I am looking forward to this PR being merged.

Just as an opinion...

I think that migration to PNPM is unnecessary, as same as adding a new "mauss" dependency. Such types of libraries should depend on as few dependencies as possible.

emersonbottero commented 2 years ago

My 2 cents.. πŸ™‚

The library is great!

emersonbottero commented 2 years ago

image

ts-thomas commented 2 years ago

@ignatiusmb thanks a lot for the big PR, I will check this very soon πŸ‘

davidhq commented 1 year ago
  • pnpm improves the development experience (saves a lot of disc space)

Have you tested this ? I actually have and it never saved any disk space.

pnpm works much faster than npm but it also breaks things a lot of times...

My 2c are that this particular change to pnpm is not advisable.

I usually use it to quickly test things and projects, when it stops working, I switch to npm... also for already mature codebase I go to npm once I don't have to reinstall modules all the time.

But space... no, I measured, it doesn't save one kilobyte but would be great if it did.

nickreese commented 1 year ago

I too would skip pnpm for a mature project but thank you for the efforts. Excited to see if it gets @ts-thomas's thumbs up or if I need to fork this branch.

lucas-labs commented 1 year ago

But space... no, I measured, it doesn't save one kilobyte but would be great if it did.

Yes it does, it uses hard links and junctions (on Windows). If you have 10 projects using the same dependency they all point to the same "global store".

https://pnpm.io/faq (first faq)

I've been using pnpm for a long time now in multiple projects, some big some small, never had a single problem that I wouldn't have had with npm or yarn. In fact, flat node_modules gives you access to dependencies that are not in your package.json, which can be problematic, pnpm helped me find this type of issues many times.

davidhq commented 1 year ago

But space... no, I measured, it doesn't save one kilobyte but would be great if it did.

Yes it does, it uses hard links and junctions (on Windows). If you have 10 projects using the same dependency they all point to the same "global store".

Ok yes, in this case it's true... if module is installed in multiple projects, it is shared... but inside one particular project, looking isolated, space is not saved... I just measured node_modules dir size inside a few projects and compared pnpm and npm and it was the same.

https://pnpm.io/faq (first faq)

I've been using pnpm for a long time now in multiple projects, some big some small, never had a single problem that I wouldn't have had with npm or yarn. In fact, flat node_modules gives you access to dependencies that are not in your package.json, which can be problematic, pnpm helped me find this type of issues many times.

That's great to hear. My experience was different. I guess it depends on luck. When this happened (and it wasn't some esoteric stuff) reinstalling with npm solved the issue... Last time I think it was with some Prisma ORM demo projects.

benmccann commented 1 year ago

These generally seem like good changes to me. I wonder if it might be easier to review if split up into separate PRs. The main question I would have is the new dependency on mauss

nosovk commented 1 year ago

it seems that mauss used only to bring tsconfig and prettier config. Probably those files could be easily moved into the project without external dependency.

ts-thomas commented 10 months ago

There is a lot of wrong intendation which results in changed lines. The project is using 4 spaces per identation scope. So I need to manually compare changes and apply them to the new version. The whole type definition is now covered by myself, but I will take your definition and merge them.

Typescript isn't an option, as long as the Closure Compiler produces more optimized Javascript. Please show me how TS can produce better code and I will change to it.

NPM is a standard, I don't see any advantage of changing it, the codebase (without dev-dependencies) are just too small.