-
-
Notifications
You must be signed in to change notification settings - Fork 491
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
modernize codebase #343
base: master
Are you sure you want to change the base?
modernize codebase #343
Conversation
@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. |
My 2 cents.. 🙂
The library is great! |
@ignatiusmb thanks a lot for the big PR, I will check this very soon 👍 |
Have you tested this ? I actually have and it never saved any disk space. pnpm works much faster than My 2c are that this particular change to 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 But space... no, I measured, it doesn't save one kilobyte but would be great if it did. |
I too would skip |
Yes it does, it uses hard links and junctions (on Windows). 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. |
Ok yes, in this case it's true... if module is installed in multiple projects, it is shared...
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 |
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 |
): Promise<boolean>; | ||
import(exported: string): this; | ||
} | ||
interface SearchOptions { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this file so much smaller now? Is it just because of formatting? It's hard to tell what changed
it seems that |
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. |
Hey, thanks for the project! I saw there's some request for help with providing types at #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
pnpm
as it's a better disk space efficient alternative tonpm
class
syntax; this is mostly an attempt to use theclass
declarator as it better encapsulates the code and logic into each place nicely without needing to hack into.prototype
-s everywhere, still a WIPPlanned TODOs
Marking this as a draft for now, @ts-thomas does this seem okay for me to continue on?