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

WIP: refactor #1115

Closed
wants to merge 1 commit into from
Closed

WIP: refactor #1115

wants to merge 1 commit into from

Conversation

komachi
Copy link
Contributor

@komachi komachi commented Mar 14, 2021

refactor: remove vendor prefixes from css/scss
refactor: get subscriptions in parallel
refactor: us fs/promises instead of sync functions from fs
refactor: replace some setTimeout calls in wrong places with better callbaack
refactor: better promise usage, cleaner code
refactor: remove unneded babel plugins


This is a little refactoring I am in progress of

Description

I removed jquery dependency. Jquery should not be used to manipulate any content vue is responsible to render, $.get and $.getJSON is replaced with moden ky, $.param replaced with small module jquery-param which implements the same algo. There is still some cases where we need to manipulate DOM (on video.js side mainly), but this could be easy solved with document.querySelector and some small helpers from shorter-js.

In some places Promises were used incorrectly, I tried to solve those cases with more cleaner code. Also I replaced sync calls to fs module with async ones.

Ideally this shouldn't broke anything, only makes codebase cleaner and an app a little bit faster

Testing (for code that is not small enough to be easily understandable)

There is still some bugs with search menu and profile selector I plan to fix later, but I want to know what do you think about this PR.

@PrestonN
Copy link
Member

Just making a comment here acknowledging that I've seen this. This is definitely a big PR so it'll take me some time to go through all of this and make sure everything is looking good. It may take a few days or so before I get to finish looking through this but just wanted to let you know that I appreciate the work that's been put into this.

refactor: remove vendor prefixes from css/scss
refactor: get subscriptions in parallel
refactor: us fs/promises instead of sync functions from fs
refactor: replace some setTimeout calls in wrong places with better callbaack
refactor: better promise usage, cleaner code
refactor: remove unneded babel plugins
fix: fix power saving suspending when video is playing, powerSaveBlocker doesn't work in renderer context
@efb4f5ff-1298-471a-8973-3d47447115dc

Hi @komachi is this still a WIP or is this ready for review?

@komachi
Copy link
Contributor Author

komachi commented Sep 20, 2021

It's still a WIP, some small features are broken, and this needs to be rebased. Feel free to cherry-pick some changes if you feel like it, as it could take a long time to get me on this PR because I'm busy with other projects (but I really plan to finish this sometime in future, no ETA).

@efb4f5ff-1298-471a-8973-3d47447115dc

No problem take ur time. I'm glad that it isn't just being abandoned. Thank you for doing such a great contribution to the FreeTube project!

@kommunarr
Copy link
Collaborator

To add on to what @efb4f5ff-1298-471a-8973-3d47447115dc said, thank you! @komachi I would also ask that you rewrite the existing history to chop the one existing large commit down into smaller, more atomized commits. This will make the work of assessing, testing, and approving your changes much easier. Again, thank you for dedication!

@ChunkyProgrammer
Copy link
Member

@absidue do you think we should close this PR now? I think we (mostly you) have implemented most of what's included already

@absidue
Copy link
Member

absidue commented Dec 20, 2022

yes, most of the changes are already implemented, the only things that could still be done is that we use sync fs calls in most places, so we could replace those with async ones in fs/promises and we still have some vendor prefixes in the css, some are necessary (some of the webkit ones) but others could be removed.

Those are rather small changes compared to this PR, so I think we can close it.

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

Successfully merging this pull request may close these issues.

6 participants