🚧 snoowrap v2.0 (work in progress)

iMrDJAi commented 3 years ago

This pull request will mainly address the following changes:

[x] axios migration. (https://github.com/not-an-aardvark/snoowrap/issues/262)
[x] The replacement of bluebird promises with async/await. (https://github.com/not-an-aardvark/snoowrap/issues/315)
[ ] Proxies removal / TypeScript issues fix. (https://github.com/not-an-aardvark/snoowrap/issues/221)
[ ] Support for more API features, undocumented endpoints / Deprecated endpoints removal. (needs more discussion)

Since most of them are considered as breaking changes, they should be released as the next major version of snoowrap "v2.0". (https://github.com/not-an-aardvark/snoowrap/issues/300)

Other changes supposed to be made before v2.0:

Bugs fixes, other necessary changes...
TypeScript migration? (https://github.com/not-an-aardvark/snoowrap/pull/214)

What I've done so far:

I've fully replaced request by axios, added support of some old request properties for a maximum backward compatibility.
I've fully removed bluebird promises from request_handler.js and used async/await instead.
Other minor changes I've mentioned in the commits messages.

What's missing?

~~Updating snoowrap.js and objects/* to match the new axios API and to replace bluebird promises. (still working on it)~~
Completing the task list at the top. ⬆⬆

I need help, I can't handle all of these alone. cc @SpyTec.

iMrDJAi commented 3 years ago

Finally! a new commit! 🎉

Most of the work is done, the axios migration is done, bluebird promises, promise-chains's promiseWrap functions, and proxies has been completely removed, some bugs are fixed and much more. See the full changes log in the latest commit message.

It still has some pending work btw, docs/readme update, more features and bug fixes, more testing, final touches...

Attention needed:

_newObject ignores most of the snoowrap#createModmailDiscussion response, no practical way to parse the returned content yet.
~~Missing method in snoowrap.js at line 1185~~ That wasn't a method.
snoowrap#composeMessage returns an empty object {} which is uncommon?
Some methods (Comment#lock, Submission#hide...) claim to return the updated version of the content (submission, comment...) even though they don't.
~~url.parse used in Listing.js at line 3 is deprecated~~ Replaced with the native URL class. Listing#_setUri() requires absolute URLs now.
PrivateMessage.js: TODO: Get rid of the repeated code here, most of these methods are exactly the same with the exception of the URIs.
Still unsure about VoteableContent#_mutateAndExpandReplies, someone should double check it.
Many unnecessary lodash functions (.map(), .forEach()...) that have native equivalents are used. @SpyTec Is that what you meant by "Remove underscore function calls maps"?

iMrDJAi commented 3 years ago

Here are other things came up to my head last night:

Unit tests have to be rewritten, they fail with latest snoowrap version, it's even worse with the upcoming 2.0 version since it contains lot of breaking changes.
The .sh scripts aren't compatible with Windows. Well, they worked with me since I have Ubuntu sub-system, but it shouldn't be hard to write Windows compatible versions.
~~The curly ESLint rule is overkill, probably we should allow inline if statements at the least, the code would be much cleaner with that.~~
Dependencies should be updated, some of them have security vulnerabilities. Also, I'm wondering why snoowrap depends on a specific commit of uglify-js instead of using a published version on the npm registry. What's so special about it?

The code is pending review and testing, @not-an-aardvark @SpyTec you should take a look. Also more discussion is needed, things must be clarified.

iMrDJAi commented 3 years ago

The first image ever to be uploaded on reddit using snoowrap
The first video ever to be uploaded on reddit using snoowrap

I'm making history! 🤩

tnm0113 commented 3 years ago

when v2.0 will be released ? so excited for this, and very good work iMrDJAi

iMrDJAi commented 3 years ago

I'm guessing that was referring to this, which is a very old backwards-compatibility measure that creates snake_case aliases like get_subreddit for all methods like getSubreddit.

@not-an-aardvark I got that! snake_case names should be definitely removed. This requires some extra work since some methods still support snake_case params.

Update: snake_case aliases are removed. However, I decided to follow a new polity:

All raw API params will be passed as is, regardless of whether they are snake_case or not. As a result, I removed camelCase aliases. This will clear confusion and keep things unified.
All other custom properties/methods/params will remain in camelCase form.

iMrDJAi commented 3 years ago

when v2.0 will be released ? so excited for this, and very good work iMrDJAi

@tnm0113 Still working on it, stay tuned!

iMrDJAi commented 3 years ago

The first poll ever to be submitted on reddit using snoowrap! 🎉
The first gallery ever to be submitted on reddit using snoowrap! 🎉
The first self post with inline media ever to be submitted on reddit using snoowrap! 🎉

I have to write docs for the new methods. 🚧🚧

iMrDJAi commented 3 years ago

~~After several attempts couldn't figure out how to include src/objects/MediaFile.ts in the documentation. I'll skip this for now.~~ JSDoc is broken with TypeScript, it needs to be replaced by TypeDoc. Or maybe we should use the Discord's docs website? In this case, I registered the organization https://github.com/snoowrap to host the docs repositories there.

https://github.com/not-an-aardvark/snoowrap/issues/283.

iMrDJAi commented 2 years ago

First, I apologize for being late. I been busy working on some side projects last month.
Anyway, this commit addresses these issues:

Missing docs for the new updates. Work in progress...

pgamerx commented 2 years ago

Any updates?

JamesxX commented 2 years ago

Typescript declaration for comments is incorrect (e.g. Snoowrap.Comment lacks a lock() method)

iMrDJAi commented 2 years ago

Yeah, while my changes are working, a lot of things are still missing/incomplete: the typescript migration, fixing docs, removing snake_case names, an allowlist for axios options, dealing with unit tests, and much more... @not-an-aardvark You should probably merge this to a development branch so more people can help and contribute. However, I'll see what I can do about that mess.

JamesxX commented 2 years ago

If there's anything I can do to help, I'd like to :) get in touch if I can!

iMrDJAi commented 2 years ago

@JamesxX Yeah, you may help me with the Typescript declarations. I need a list of missing/incorrect declarations so I can fix them. However, I still want to migrate the whole library to Typescript, are you up to that?

JamesxX commented 2 years ago

Regarding axios migration, the unit test still needs the request-promise library in a few places. It wouldn't take much to upgrade that to axios too, but I'm not familiar enough with either sadly.

https://github.com/not-an-aardvark/snoowrap/blob/master/test/snoowrap.spec.js#L8 https://github.com/not-an-aardvark/snoowrap/blob/master/test/snoowrap.spec.js#L49-L65 https://github.com/not-an-aardvark/snoowrap/blob/master/test/snoowrap.spec.js#L196-L222

KilianB commented 2 years ago

When bumping es lint you need to update the eslintrc

https://github.com/iMrDJAi/snoowrap/blob/8901aa5fc32c65fb2ee6b1f6f0633db95ca60810/.eslintrc.yml#L109 ''@typescript-eslint -> @typescript-eslint/eslint-plugin'

ws currently has critical security vulnerabilities.

Version 6 drops support for browser bundles : https://github.com/websockets/ws/releases/tag/6.0.0

Version 8 has breaking changes which require changes in LiveThread.js

    const handler = (data, isBinary) => {
    const parsed = this._r._populate(JSON.parse(isBinary ? data.toString() : data));

and snoowrap.js

        ws.onmessage = (eventData, isBinary) => {
          ws.onclose = null;
          ws.close();
          const data = JSON.parse(isBinary ? eventData.toString() : eventData);

KilianB commented 2 years ago

I created my own fork with the changes and swapped out request in the test file, couldn't run the test thought since I do not have the oauth.json file available. The ws version push will make this change not compatible with the goal of this repo but since I only care about running it server side maybe you can take a few hints.

https://github.com/KilianB/snoowrap/tree/update_dependencies

iMrDJAi commented 2 years ago

When bumping es lint you need to update the eslintrc

@KilianB I use @typescript-eslint instead of @typescript-eslint/eslint-plugin and everything seems to work fine. Are you sure this is needed?

ws currently has critical security vulnerabilities.

Noted! I'll deal with the breaking changes.

Version 6 drops support for browser bundles

That's not an issue, ws is only being used with node. On browser environments, I'm using the native WebSocket class.

couldn't run the test thought

The tests are broken even with snoowrap 1.23.0, they need a full re-write.

I suggest checking dev2 branch that includes my latest changes instead.

KilianB commented 2 years ago

When bumping es lint you need to update the eslintrc

@KilianB I use @typescript-eslint instead of @typescript-eslint/eslint-plugin and everything seems to work fine. Are you sure this is needed?

I am not sure and according to the docs @typescript-eslint should be enough. For me changing it fixed the issue but maybe it was just dependencies playing along badily

ws currently has critical security vulnerabilities.

Noted! I'll deal with the breaking changes.

Version 6 drops support for browser bundles

That's not an issue, ws is only being used with node. On browser environments, I'm using the native WebSocket class.

Great :)

I suggest checking dev2 branch that includes my latest changes instead.

Thank you very much!

SuperchupuDev commented 2 years ago

As of Node 18, native fetch() has been implemented, which removes the need for third party libraries, I think it would be a good idea to use it instead of axios, however that would drop support of the library for node versions <18. Thoughts?

iMrDJAi commented 2 years ago

As of Node 18, native fetch() has been implemented, which removes the need for third party libraries, I think it would be a good idea to use it instead of axios, however that would drop support of the library for node versions <18. Thoughts?

@SuperchupuDev That's not a good idea, we can't just drop support for node <18. Also, I'm willing to support older browsers that don't even support fetch natively. Compatibility with older environments is important.

Chaphasilor commented 2 years ago

As of Node 18, native fetch() has been implemented Thoughts?

How about using the native implementation if available, and polyfilling with node-fetch pre-Node 18?
The API should be compatible.

Chaphasilor commented 2 years ago

Compatibility with older environments is important

Is it though? I mean, it's not like older versions of snoowrap would stop working :)
But developing new features with deprecated browsers in mind is not a good idea, imo.

iMrDJAi commented 2 years ago

How about using the native implementation if available, and polyfilling with node-fetch pre-Node 18? The API should be compatible.

@Chaphasilor Sorry for the late reply.

Are there any actual benefits to using native fetch? In other words, I know it provides much better performance, but, is that critical?

Is it though? I mean, it's not like older versions of snoowrap would stop working :) But developing new features with deprecated browsers in mind is not a good idea, IMO.

It's not about the browsers, it's about providing a better experience for developers, take me as an example. :)
I build Android apps using Cordova, it relies on Android Webview to work. Some devices ship with an unupgradable old version of Webview, See?
And no, I don't support deprecated browsers, and absolutely not Internet Explorer :3

Also, keep in mind that I already have a working implementation of snoowrap based on axios.

Chaphasilor commented 2 years ago

The performance isn't the advantage here, but getting rid of a major dependency. Remember request?

Do these old webviews really not support fetch? CanIUse shows pretty good coverage...

You do have a valid point with regards to the existing implementation of course :)
If it's too much work, we can stop discussing \^\^

SuperchupuDev commented 2 years ago

It indeed has really good coverage, MDN says fetch() has been supported in all major browsers (except Internet Explorer) since like 2015 (with the exception of Safari being in 2017) https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API#browser_compatibility

iMrDJAi commented 2 years ago

@SuperchupuDev Unfortunately, Android 5.1 (The lowest version that a sane developer would care about) ships with WebView 39.

SuperchupuDev commented 2 years ago

Damn that's sad, is it upgradable at least? (even then Android 5 was released in 2014 and I rarely see someone using it these days, affected users could use a web browser anyways)

luc122c commented 2 years ago

Hi all, it's an interesting discussion, i'd like to chime in with a few thoughts.

Firstly, you're right, the support tables for Fetch and Promises are almost identical. Snoowrap 2.0 is already a breaking change, therefore developers should carefully weigh up the benefits of the upgrade with the changes they may need to make to their code. If it is made clear that the supported platforms are also changing, I don't see that developers would take issue with this.

With regard to Node support, v18 is now current. Since snoowrap 2.0 is a breaking change, now would be the best time to make the switch. Node v16 moves out of Active LTS October 22 which is only 5 months away, i'm not sure what the expected release of snoowrap 2.0 is but I imagine it could be near this date.

As for the benefits of using Promises and Fetch, I concur that performance is not the only gain. Documentation, standardisation, support and dependency reduction should also be considered. Native features are widely used and well documented meaning there is plenty of support available. Furthermore, reducing dependencies is always a huge win, as it reduces the risk of supply chain attacks, reliance on an unknown developer and any decisions they may make.

That all being said, Axios is a highly popular and well-documented tool; I can't see it going away any time soon without major kickback from the community. As a replacement for Request, it's great and the work done so far to migrate away from Request is fantastic. However, bear in mind that this had to happen because Request did go away; it's not too far fetched for the same to happen to Axios.

Lastly, thank you for your incredible work on this library. It is definitely appreciated!

SuperchupuDev commented 2 years ago

Absolutely agree with the above comment, await ships in a greater version of Android Webview (55) than fetch (42) and it's already in v2.0, in fact every major browser shipped fetch earlier than await, I don't really think using native fetch() would drop compatibility outside of Node https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/await#browser_compatibility

iMrDJAi commented 2 years ago

is it upgradable at least?

@SuperchupuDev It's not if com.android.webview, however com.google.android.webview is upgradable. BTW, currently I'm using an Android 5.1 device, it's shame that the manufacturers aren't producing small-sized smartphones anymore. :")

@luc122c @SuperchupuDev I'm not against using new features. In fact that won't usally affact older environments since the code gets transpiled at the end (e.g. async/await to native promises).
The features that should be avoided are the ones that may not work on the lagecy environments even after transpiling the code (e.g. the new regex syntax). Or those that may cause a bundle size increasment (e.g. polyfills).
Axios on browser is based on the XMLHttpRequest API which is widly suppored, also it has a powerful API and it's way easier to build wrappers than Fetch, this is why I decided to stick to it.
Also, dropping node <8 versions is a stupid idea, not everyone is going to upgrade. Regardless of the fact that the available binaries of node for the mobile platforms (nodejs-mobile) are still in v12.

iMrDJAi commented 2 years ago

I'm busy with work latly. Are there any TypeScript developers who are intersted to help finishing the megration? The more the better! (cc @JamesxX).

@not-an-aardvark @SpyTec and the other contributors, I think it's the time for you to act, we need to speed it up, I need help.

iMrDJAi commented 2 years ago

Closing due to the inactivity of maintainers. Feel free to reach me out if you're interested to open it again.

Venefilyn commented 2 years ago

Hobby coding has been on the back burner over the past year on my end due to both professional and personal reasons. There's two big drawbacks to snoowrap that causes such slow development, both maintainers are relatively inactive to the project, and lack of community engagement.

I can't at this time make any commitments in the near future, neither to this project or any of the others I'm a maintainer of. I hope we can find some people willing to continue the work

What I think would be best is to merge to a new branch for v2 development, document what has been done and what is missing in the tracking issue, and split the tasks into smaller things. Once it is believed there is a working prototype I can at least release pre-release versions of v2 in npm.

Outstanding for this PR would be to have it reviewed. But, I can't commit to doing it myself in the coming weeks

iMrDJAi commented 2 years ago

Hobby coding has been on the back burner over the past year on my end due to both professional and personal reasons.

Unfortunately, this is the case for me too. I found myself unable to continue working on it without help, I was slow and I couldn't achieve progress in a reasonable amount of time.

document what has been done and what is missing in the tracking issue, and split the tasks into smaller things

I'll take a look on my work again and document everything, then I'll open a new pull request instead to keep it clean.

amosbastian commented 2 years ago

Hey, is there any way I can help with this?

DedaDev commented 1 year ago

I'll take a look on my work again and document everything, then I'll open a new pull request instead to keep it clean.

@iMrDJAi how its going with this?

What I think would be best is to merge to a new branch for v2 development, document what has been done and what is missing in the tracking issue, and split the tasks into smaller things. Once it is believed there is a working prototype I can at least release pre-release versions of v2 in npm.

@SpyTec please do it, make v2 branch, @iMrDJAi should re-direct this PR to that branch, and let the others contribute, so we can move on with this.

iMrDJAi commented 1 year ago

@DedaDev To be honest I didn't have enough time to get back to this project in the last few months, unfortunately I made 0 progress since then. I still have interest but.. not time.

@SpyTec please do it, make v2 branch, @iMrDJAi should re-direct this PR to that branch, and let the others contribute, so we can move on with this.

I can't agree more, I think merging this to a v2 branch would speed it up, this will allow more developers to contribute.

Venefilyn commented 1 year ago

v2.0 created and PR changed to point to it. If people want to take a look at this PR and give their review feel free, then I can merge sometime next week - just to give everyone more time to note down what's missing overall

SuperchupuDev commented 1 year ago

Now that Node v18 is LTS I think this would be a good time to switch to the built-in fetch API, Node v14 is reaching EOL soon and v12 already did a while ago (iirc the Fetch API also got backported to v16), why supporting old versions that aren't officially supported anymore? If someone really needs support for legacy environments they can just keep using v1 🤷‍♀️

iMrDJAi commented 1 year ago

@SuperchupuDev I dislike these EOL policies tbh, in reality they just don't work, following them blindly would bring nothing but unnecessary limitations. You only drop support for legacy environments if your project cannot be kept functional on these environments anymore. Learn how to make your code platform independent.

iMrDJAi commented 1 year ago

v2.0 created and PR changed to point to it. If people want to take a look at this PR and give their review feel free, then I can merge sometime next week - just to give everyone more time to note down what's missing overall

Here is a hint: most likely there are remaining files that need to be ported to TypeScript, also there are still some lodash function calls that need to get replaced by native solutions.

DedaDev commented 1 year ago

There is the way too many things to be fixed, I think you took a huge bite @iMrDJAi Subreddit.ts is 1560 lines long, I don't want to even look at it.

Let's start small, I would suggest fixing the current code first, so fixing all tests, and updating the API-s. Then replacing the request-promise with Axios. Then we can think of typescript and all the fancy stuff. Add me on discord to coordinate around this, Deda#6991

Venefilyn commented 1 year ago

Lots of things to do still, we know that. I merged the branch, anything else can be brought up in #300 or subsequent PRs.

While this PR took a long time, since we have a lot of participants interested please take a look at Issue #370 about the future of snoowrap. The project has no velocity and something needs to change

DedaDev commented 1 year ago

Nooo, revert the merge please @SpyTec . That PR has 200+ errors, tests are broken, no one would be able to continue that work, we need smaller tasks. I like @iMrDJA enthusiasm, he wanted to fix everything, but as I said, he took a huge bite that he cannot swallow, that usually leads to unfinished job. We need smaller cycles like with scrum (Agile).

And lets make a rule that only PRs that are passing all tests can be merged. (husky would be a good implementation)

Venefilyn commented 1 year ago

Nooo, revert the merge please @SpyTec . That PR has 200+ errors, tests are broken, no one would be able to continue that work, we need smaller tasks. I like @iMrDJA enthusiasm, he wanted to fix everything, but as I said, he took a huge bite that he cannot swallow, that usually leads to unfinished job. We need smaller cycles like with scrum (Agile).

Several times the PR has been asking for contributions from other people. When I set out the v2.0 tracking issue I didn't expect one giant PR either, but if someone already did a lot of code I will let them continue. With hobby projects with little activity you have to take some liberties in how things are handled

And lets make a rule that only PRs that are passing all tests can be merged. (husky would be a good implementation)

We do not put broken code to main branch, all tests have to pass already. This was merged into a separate branch to make collaboration easier

SuperchupuDev commented 1 year ago

just wondering, what is the minimum node version planned to be supported in 2.0? looks like the ci is failing because it's using node 8.15 which breaks since esbuild uses } catch { which was added in node 10. i would suggest supporting v12+ but i'm not the one to decide that

DedaDev commented 1 year ago

@SpyTec he said that he doesn't have time to work on it, https://github.com/not-an-aardvark/snoowrap/pull/338#issuecomment-1294848119 and everyone is going to be super lost when they open up the code as I was.

FoxxMD commented 1 year ago

@DedaDev I think you're missing the point...this PR and the branch 2.0 consist solely of DJA's commits, based off the latest master branch. There is no other "2.0" that was merged into DJA's PR. It's just their work.

You can work from that if you want. Or you can start from the master branch. Or you can branch off some earlier commit in the 2.0 branch. If you want to PR another "next" branch based off of only a few commits and go step by step you can do that too.

The point is this work is now available in this repo for others to discuss and PR off of, instead of having to restrict everything to this one PR discussion or having to move the discussion to DJA's fork.

iMrDJAi commented 1 year ago

@DedaDev to be clear, Subreddit.js and other files from v1 were already big, also the code base was legacy and much complex than the one on this PR (replacing bluebird promises helped), so at least now we have modern code to fix. I stated before that the tests are totally broken with v2 and they need to be written from scratch so don't worry about the results.

I don't disagree with what you said, the PR was too huge the way I lost track of what I was doing, also I wasn't able to find enough time to continue, that's on me. I don't promise anything but I'll give it a shot again. I may reach you out on Discord this weekend.

Merging this doesn't mean anything but allowing more developers to contribute, it will never be released if it remains in this state.

not-an-aardvark / snoowrap

🚧 snoowrap v2.0 (work in progress) #338