🚧 snoowrap v2.0 (work in progress) #338
Conversation
I'm a windows user '-'
- ➕ Added axios, form-data. - ➕ Added live-server to simplify running tests on browser. - 🔨 Updated request_handler.js: - 🔥 Removed bluebird Promises to use async/await instead. - 💥 Replaced request with axios. - 👽️ Updated requests options to match the axios API. - 🚚 Renamed xhr.js to axios.js. - 🔨 Updated axios.js: - 🎨 Added request interceptor to modify requests options. - 👽️ Added support to some old request options for a maximum backward compatibility. - 🚨 Removed ESLint promise/no-native rule. (some rules are overkill, consider removing some of them to use a newer JavaScript syntax).
- 📝 Updated docs. - 🔨 Updated axios.js: - 🎨 Improved requests options validation. - ✨Added a way to debug requests/responses internally. - 🔨 Updated request_handler.js: - 🐛Fixed a bug with errors handling in `snoowrap#oauthRequest`. - ✨Added `npm run browser` to simplify testing and debugging on browser console.
🚨 Removed `"use strict";` on `browser/index.js` since it causes linter issues. 🩹 Added an additional check for the debug function since it may not always be available on `_r`.
- 🔨 Updated `snoowrap#getAuthUrl`: - ✨The default scope now is set to `['*']` which gives you all permissions. - ✨Added support for the `compact` mode which represents the mobile version of the authorization URL. - 🐛 Fixed a bug with `snoowrap#fromAuthCode` and `snoowrap#fromApplicationOnlyAuth` when they don't set the values of `requester.tokenExpiration` and `requester.scope` after a successful authentication which causes the access token to refresh again for no reason '-' . - 🐛 Fixed the broken `snoowrap#markAsVisited` method (I havn't test it yet bcs I don't have reddit premium). - 🐛 Fixed a bug with `snoowrap#getRandomSubmission` when the reddit server returns the subreddit itself instead of a random submission. - 🩹 Now the `scope` value is getting removed from the requester object when calling `snoowrap#revokeAccessToken` or `snoowrap#revokeRefreshToken`. - 👽️ Updated all methods to match the new `axios` API. - ➖ Removed `promiseWrap` and `proxies`. - 🎨 More clean code. - 💡 More clean comments. - 📝 Updated docs.
|
Finally! a new commit! 🎉 Most of the work is done, the It still has some pending work btw, docs/readme update, more features and bug fixes, more testing, final touches... Attention needed:
|
|
Here are other things came up to my head last night:
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. |
- ✨Added image/video/videogif submissions support! `snoowrap#submitImage`, `snoowrap#submitVideo` and `snoowrap#_uploadMedia`. (Thx PRAW 😂) - 🐛 Fixed an issue with `FormData` on `axios.js` when running snoowrap on node. - 📝 Updated docs.
|
when v2.0 will be released ? so excited for this, and very good work iMrDJAi |
There was a problem hiding this comment.
Thanks very much for all your work on this! I haven't finished going through all the changes closely yet, but from skimming it briefly I left a couple comments. I still have some more reviewing to do.
Re:
Many unnecessary lodash functions (_.map(), _.forEach()...) that have native equivalents are used. @SpyTec Is that what you meant by "Remove underscore function calls maps"?
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! Update: snake_case aliases are removed. However, I decided to follow a new polity:
|
@tnm0113 Still working on it, stay tuned! |
- ✨ Added `snoowrap#submitGallery`. - ✨ Added `snoowrap#submitPoll`. - ✨ Added inline media support for `snoowrap#submitSelfpost`. - ✨ Added `snoowrap#_convertToFancypants`. Used to convert `markdown` to `rtjson` format for the `fancy pants` editor. Necessary to embed inline media in selfposts. - 🔨 Improved validation for `snoowrap#submitVideo` . - ✨ Created `MediaFile.ts`. It contains classes that represent uploaded media can be used inline or with other `snoowrap#submitXXXX` functions. - 🔨 Updated `snoowrap#_uploadMedia` to return instances of `MediaFile`. - 🚧 Missing docs..
|
The first poll ever to be submitted on reddit using snoowrap! 🎉 I have to write docs for the new methods. 🚧🚧 |
- 📝 Updated docs. - 🚧 Missing docs for `src/objects/MediaFile.ts`.
|
|
|
First, I apologize for being late. I been busy working on some side projects last month.
Missing docs for the new updates. Work in progress... |
- 📝 Updated docs. - 🐛 Bug fixes and logic improvements.
🔨 Added shortcuts of the new `submit` methods to the subreddit object
|
Any updates? |
|
Typescript declaration for comments is incorrect (e.g. Snoowrap.Comment lacks a lock() method) |
|
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 |
|
If there's anything I can do to help, I'd like to :) get in touch if I can! |
|
Absolutely agree with the above comment,
|
@SuperchupuDev It's not if @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). |
|
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. |
|
Closing due to the inactivity of maintainers. Feel free to reach me out if you're interested to open it again. |
|
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 |
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.
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. |
|
Hey, is there any way I can help with this? |
@iMrDJAi how its going with this?
@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. |
|
@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.
I can't agree more, I think merging this to a v2 branch would speed it up, this will allow more developers to contribute. |
Merging dev2...
|
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 |
|
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 🤷♀️ |
|
@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. |
Here is a hint: most likely there are remaining files that need to be ported to TypeScript, also there are still some |
| @@ -19,18 +19,18 @@ rules: | |||
| indent: [error, 2, {FunctionDeclaration: {body: 1, parameters: 1}, FunctionExpression: {body: 1, parameters: 1}, CallExpression: {arguments: 1}, SwitchCase: 1, MemberExpression: 1}] | |||
There was a problem hiding this comment.
| indent: [error, 2, {FunctionDeclaration: {body: 1, parameters: 1}, FunctionExpression: {body: 1, parameters: 1}, CallExpression: {arguments: 1}, SwitchCase: 1, MemberExpression: 1}] | |
| "@typescript-eslint/indent": [error, 2, {FunctionDeclaration: {body: 1, parameters: 1}, FunctionExpression: {body: 1, parameters: 1}, CallExpression: {arguments: 1}, SwitchCase: 1, MemberExpression: 1}] |
There was a problem hiding this comment.
@DedaDev I believe we should disable all formatting rules and install prettier instead.
| * instantiate it directly. | ||
| * <style> #RedditContent {display: none} </style> | ||
| */ | ||
| class RedditContent<T extends RedditContent = RedditContent<any>> { |
There was a problem hiding this comment.
I'm unsure what you are doing here, making generics for the class in which T extends the Class itself?
and you are not even using that T in the body
There was a problem hiding this comment.
It helps constrain the generic when extending other classes.
There was a problem hiding this comment.
@DedaDev We pass sub classes as generic types to RedditContent<> to ensure that methods like fetch() and refresh() return the correct typing. RedditContent<any> is the default in case you do new RedditContent().
| export {default as requiredArg} from './requiredArg' | ||
| export {default as URL} from './URL' | ||
| export {default as URLSearchParams} from './URLSearchParams' | ||
| export {default as WebSocket} from './WebSocket' |
There was a problem hiding this comment.
maybe named export would suit better here?
There was a problem hiding this comment.
@DedaDev Idk.. Yeah, this file isn't necessary.
| semi: [error, always] | ||
| semi-spacing: error | ||
| # semi: [error, never] | ||
| # semi-spacing: error |
There was a problem hiding this comment.
@DedaDev To temporarily suspense linter error till I remove all semicolons.
| username: string | ||
| password: string | ||
| two_factor_code?: number | string | ||
| access_token?: string |
There was a problem hiding this comment.
@DedaDev Well, there are too many files in the project already, not sure if creating more is a good idea.
|
There is the way too many things to be fixed, I think you took a huge bite @iMrDJAi Let's start small, |
|
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 |
|
Nooo, revert the merge please @SpyTec . That PR has 200+ errors, tests are broken, no one would be able to continue that work, And lets make a rule that only PRs that are passing all tests can be merged. (husky would be a good implementation) |
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
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 |
|
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 |
|
@SpyTec he said that he doesn't have time to work on it, #338 (comment) |
|
@DedaDev I think you're missing the point...this PR and the branch 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. |
|
@DedaDev to be clear, 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. |
This pull request will mainly address the following changes:
axiosmigration. (Request has been deprecated #262)The replacement of
bluebirdpromises withasync/await. (Any plans to switch to ES6 promises rather than Bluebird's? #315)Proxies removal / TypeScript issues fix. (Type error with async/await #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". (#300)
Other changes supposed to be made before v2.0:
What I've done so far:
requestbyaxios, added support of some oldrequestproperties for a maximum backward compatibility.bluebirdpromises fromrequest_handler.jsand usedasync/awaitinstead.What's missing?
Updatingsnoowrap.jsandobjects/*to match the newaxiosAPI and to replacebluebirdpromises. (still working on it)I need help, I can't handle all of these alone. cc @SpyTec.