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

Collaborate? #11

Closed
sindresorhus opened this issue Aug 4, 2022 · 47 comments
Closed

Collaborate? #11

sindresorhus opened this issue Aug 4, 2022 · 47 comments

Comments

@sindresorhus
Copy link

I noticed you have done a lot of great improvements to Caprine in this fork. Would you be interested in collaborating on the official repo? I would be happy to add you as a maintainer if so.

@sindresorhus
Copy link
Author

Same offer for @1nikolas.

@sindresorhus
Copy link
Author

sindresorhus commented Aug 4, 2022

And let me know if there's anything that prevents you from contributing upstream. I'm happy to work with you to resolve any issues.

@1nikolas
Copy link
Collaborator

1nikolas commented Aug 4, 2022

Would be very happy to help improve the official repo. Count me in!

(I'm currently on vacation so I can't do much now)

@lefterisgar
Copy link
Owner

Hi there! I accept the offer too and I'm really happy you appreciate our work. I think @1nikolas is a very skilled developer and he has helped in a lot of things including upgrading Electron. I will be taking a break from school so I guess I can do some contributions.

Caprine is an awesome project and I don't want it to die. Upstream has some issues due to an old version of Electron and upgrading that has a lot of breaking changes. That's why I initially forked the project.

@1nikolas
Copy link
Collaborator

1nikolas commented Aug 4, 2022

And let me know if there's anything that prevents you from contributing upstream. I'm happy to work with your to resolve any issues.

There is this one module of yours which is being used as a dependency of a dependency on Caprine. Due to electron remote changes on recent electron versions, it needs to be patched (I'm currently using a patching node module). I'll tell you more about it when I return from vacation

@sindresorhus
Copy link
Author

Awesome :)

I have added you to the repo.

@lefterisgar
Copy link
Owner

Well thank you soo much! I will help the project to the best of my ability. I'm closing this issue now.

@sindresorhus
Copy link
Author

I have cherry-picked most of the relevant commits: https://github.com/sindresorhus/caprine/commits/main

@sindresorhus
Copy link
Author

Some quick feedback, with sindresorhus@8fd711e, I think it would be smart to add a TODO comment to each visible: is.development, about removing it when the feature is fixed.

@lefterisgar
Copy link
Owner

I have cherry-picked most of the relevant commits: https://github.com/sindresorhus/caprine/commits/main

Oh, what you've done is great! Now it will be easier from me to start working right from where you left!

@lefterisgar
Copy link
Owner

Some quick feedback, with sindresorhus@8fd711e, I think it would be smart to add a TODO comment to each visible: is.development, about removing it when the feature is fixed.

Yeah, why not? While you're at it, some features (IMO) seem stupidly easy to fix, I just didn't have the time to check.

@1nikolas
Copy link
Collaborator

1nikolas commented Aug 4, 2022

@lefterisgar now I think we need to make some quality of life things like:

  • Decluttering issues
  • Actually fixing them (or close as wontfix)
  • Removing unnecessary libraries and keeping as little as possible (so it's easier to maintain and the app runs faster). Most of sindresorhus' libraries can be replaced with simpler/less code (no offense for the libraries but it makes more sense not to use them in this simple app)
  • Coming up with a better way to manage html class names (eg put them in a separate file with consts) so everyone can contribute those without needing any code knowledge. Class names change from time to time and are really easy to find

@sindresorhus
Copy link
Author

Most of sindresorhus' libraries can be replaced with simpler/less code (no offense for the libraries but it makes more sense not to use them in this simple app)

Which libraries specifically? I'm not offended at all.

Keep in mind that it's a common programming fallacy to look at something complicated and think you could rewrite it simpler, but what's often missed is that these libraries probably started out simple too, but got complicated because they had to handle real world situations and edge-cases.

@lefterisgar
Copy link
Owner

@1nikolas I totally agree with your suggestions. For the html class names part, we could also come up with a better way that doesn't break whenever Facebook/Meta decides to do a minor UI change. Also a good idea would be to open an issue (or better yet enable discussions) so we can create a roadmap for the next release which is going to be big.

@1nikolas
Copy link
Collaborator

1nikolas commented Aug 4, 2022

Which libraries specifically? I'm not offended at all.

Keep in mind that it's a common programming fallacy to look at something complicated and think you could rewrite it simpler, but what's often missed is that these libraries probably started out simple too, but got complicated because they had to handle real world situations and edge-cases.

I strongly agree with all those but I don't think for example a library to generate a github issues link is required. Remember, electron is way to slow by it's own, so we just need to use as little libraries as possible if want to make it "snappier". A good example is openasar for discord which makes it way faster by removing all it's dependencies.

Edit: Maybe not most of them but some seem to me unnecessary

@lefterisgar
Copy link
Owner

@sindresorhus Another issue is that a lot of npm modules you have created and included in Caprine, are no longer compatible with it. We have to use older versions of them which have not received any security fix in the past 3 years or so. An example would be @sindresorhus/do-not-disturb. Latest version is 2.1.0 but we are stuck with 1.1.0 and upgrading it breaks Caprine.

@1nikolas
Copy link
Collaborator

1nikolas commented Aug 4, 2022

@sindresorhus Another issue is that a lot of npm modules you have created and included in Caprine, are no longer compatible with it. We have to use older versions of them which have not received any security fix in the past 3 years or so. An example would be @sindresorhus/do-not-disturb. Latest version is 2.1.0 but we are stuck with 1.1.0 and upgrading it breaks Caprine.

Yeah, do not disturb also needs Windows and Gnome/KDE support so this would make sense to just rewrite. I have experience with all 3 OSes and their scripts

@lefterisgar
Copy link
Owner

lefterisgar commented Aug 4, 2022

Yeah, do not disturb also needs Windows and Gnome/KDE support so this would make sense to just rewrite

Are you sure that a newer version doesn't support it? It hasn't been updated in the last 3 years!

@sindresorhus
Copy link
Author

I strongly agree with all those but I don't think for example a library to generate a github issues link is required.

It's definitely not required, but it made it more convenient for me. I'm not against removing it.

Remember, electron is way to slow by it's own, so we just need to use as little libraries as possible if want to make it "snappier".

Removing such libraries will have no noticable impact on the performance though. The thing that makes Caprine slow, or the thing that makes many Electron apps slow, is too much IPC (communication between main and renderer). IPC on Electron is laughably slow. And @electron/remote makes use of a lot of IPC. So the best way to make Caprine actually faster is to slowly get rid of @electron/remote. That's also why Electron deprecated the built in remote API.

@sindresorhus
Copy link
Author

An example would be @sindresorhus/do-not-disturb. Latest version is 2.1.0 but we are stuck with 1.1.0 and upgrading it breaks Caprine.

Wouldn't help. The latest version of that package is broken too in a different way: sindresorhus/do-not-disturb#12

@lefterisgar
Copy link
Owner

Oh, sorry, I didn't know.

@lefterisgar
Copy link
Owner

Is rewriting the entire package worth the effort?

@sindresorhus
Copy link
Author

Another issue is that a lot of npm modules you have created and included in Caprine, are no longer compatible with it. We have to use older versions of them which have not received any security fix in the past 3 years or so.

That will be solved by Electron eventually. I do backport security fixes to my packages. I don't think any of the "security issues" apply to an Electron app anyway, they're about server stuff, like ReDoS.

@sindresorhus
Copy link
Author

Is rewriting the entire package worth the effort?

We could use https://github.com/arodik/macos-focus-mode, but we would have to include instructions to install the shortcut.

@lefterisgar
Copy link
Owner

Another issue is that a lot of npm modules you have created and included in Caprine, are no longer compatible with it. We have to use older versions of them which have not received any security fix in the past 3 years or so.

That will be solved by Electron eventually. I do backport security fixes to my packages. I don't think any of the "security issues" apply to an Electron app anyway, they're about server stuff, like ReDoS.

They might be keeping up other vulnerable packages from updating though. When I finally managed to upgrade one of them it solved quite a few vulnerabilities.

@1nikolas
Copy link
Collaborator

1nikolas commented Aug 4, 2022

Removing such libraries will have no noticable impact on the performance though. The thing that makes Caprine slow, or the thing that makes many Electron apps slow, is too much IPC (communication between main and renderer). IPC on Electron is laughably slow. And @electron/remote makes use of a lot of IPC. So the best way to make Caprine actually faster is to slowly get rid of @electron/remote. That's also why Electron deprecated the built in remote API.

Isn't your utils lib doing that a lot?

@1nikolas
Copy link
Collaborator

1nikolas commented Aug 4, 2022

Is rewriting the entire package worth the effort?

We could use https://github.com/arodik/macos-focus-mode, but we would have to include instructions to install the shortcut.

Isn't it possible with applescript? I've done this before (using applescript from electron). Also why do we need a do not disturb library? Isn't it automatically muting notifications when in dnd?

@sindresorhus
Copy link
Author

Isn't your utils lib doing that a lot?

Yes.

So in short; I made a lot of Electron packages to solve my needs, then the Electron team decided to introduce a lot of breaking changes, including removing remote, and I had to completely rewrite all my Electron packages. I just never had time to rewrite electron-util. Back then when I made those packages, it was not generally known that the Electron IPC was so slow.

There has been an attempt at rewriting it: sindresorhus/electron-util#58, but I don't know that status of it. // @dusansimic

@sindresorhus
Copy link
Author

Isn't it automatically muting notifications when in dnd?

Yes. I believe it's to silence sound, dock badge, and dock bouncing.

@sindresorhus
Copy link
Author

Isn't it possible with AppleScript?

I don't remember. I think it broke in some macOS version.

@lefterisgar
Copy link
Owner

lefterisgar commented Aug 4, 2022

@sindresorhus Would you mind if I squashed some of the commits so the history looks cleaner? Also, it looks like some deps haven't been bumped.

@1nikolas
Copy link
Collaborator

1nikolas commented Aug 4, 2022

Isn't it automatically muting notifications when in dnd?

Yes. I believe it's to silence sound, dock badge, and dock bouncing.

So this would be applicable on other OSes too.

Anyway, is there any proper way we can 3 of us (and all the other maintainers) talk? Filling up a GitHub issue on this forked repo doesn't seem convenient. Maybe github's discussions?

@lefterisgar
Copy link
Owner

lefterisgar commented Aug 4, 2022

@1nikolas Exactly! I also suggested to enable discussions. I use Telegram if anyone is interested.

@1nikolas
Copy link
Collaborator

1nikolas commented Aug 4, 2022

@1nikolas Exactly! I also suggested to enable discussions. I use Telegram if anyone is interested.

My telegram is on my GitHub profile

@dusansimic
Copy link

Anyway, is there any proper way we can 3 of us (and all the other maintainers) talk?

Exactly what i thought. I have most social networks so anything is fine by me but i prefer open platforms (Matrix or XMPP). GitHub discussion are also a good suggestion but i think issues are fine for now, at list if they're on the upstream repo.

Regarding the electron-util package, I'll write an answer tomorrow.

@dusansimic
Copy link

The status is the following. I think I've completed the rewrite but I still have some issues when I tried the library out on electron. When I get back from vacation I could post all the details in the PR discussion. One thought I had was to rewrite the library in typescript so type definitions would be in by default and we wouldn't need to maintain two things. Also I wanted to switch from using "main" prop for exporting the interface to "exports" since i found it easier to export interfaces and test them at the same time.

As I said, I'll write up everything once I get back from vacation (second part of august).

@sindresorhus
Copy link
Author

Would you mind if I squashed some of the commits so the history looks cleaner? Also, it looks like some deps haven't been bumped.

Sure. Just make sure you use git push --force-with-lease so it doesn't overwrite any new commits.

I didn't include all the dependency bumps as they had conflicts. I think we can do a single commit that bumps all the dependencies.

@sindresorhus
Copy link
Author

One thought I had was to rewrite the library in typescript so type definitions would be in by default and we wouldn't need to maintain two things.

👍

@lefterisgar
Copy link
Owner

I didn't include all the dependency bumps as they had conflicts. I think we can do a single commit that bumps all the dependencies.

Oh, I didn't know. They should be bumped now, anyways. I have also fixed the sidebar and I'm working now to fix the linter errors.

@sindresorhus
Copy link
Author

The linter errors are fixed: sindresorhus@1c10a52

@lefterisgar
Copy link
Owner

Great!!!

@lefterisgar
Copy link
Owner

It looks like a certificate for macOS is expired.

@lefterisgar
Copy link
Owner

sindresorhus@1c10a52 breaks dark mode scrollbar.
εικόνα

@lefterisgar
Copy link
Owner

Should be fixed in sindresorhus@72ae40b

@dusansimic
Copy link

I'd like to start of communication in any way possible. Mainly because there were some commit that I don't quite understand why they are there. We could begin with emails (you could contact me via my email from my profile page (dusan.simic1810@gmail.com) and I'll create an initial email thread) and then we could establish some basic layout for the development process.

Caprine is is a bit bulky and it would be ideal if we could have some kind of a review process like in pull requests on the upstream repo. I'm saying this so unwanted commits and changes could be caught before they are pushed on the main branch.

@lefterisgar
Copy link
Owner

I've just sent you an email.

@lefterisgar
Copy link
Owner

lefterisgar commented Aug 16, 2022

Now the CI fails again. As I said earlier, enabling snap as a target is probably what caused it. Previously, it failed on ubuntu-latest, now it fails on windows-latest.

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

No branches or pull requests

4 participants