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

Bloxy Version 6 #287

Draft
wants to merge 26 commits into
base: v5
Choose a base branch
from
Draft

Bloxy Version 6 #287

wants to merge 26 commits into from

Conversation

Syrilai
Copy link
Collaborator

@Syrilai Syrilai commented Jan 7, 2022

This PR (and branch) updates bloxy to version 6.

No information yet to this other than it will fix a lot of problems.

To-Do:

  • Update APIs

APIs:

  • AccountInformation
  • AccountSettings
  • AdConfiguration
  • AssetDelivery
  • Auth
  • Avatar
  • Badges
  • Base
  • Billing
  • Catalog
  • Chat
  • Contacts
  • Data
  • Develop
  • Economy
  • EconomyCreatorStats
  • EngagementPayouts
  • Followings
  • Friends
  • GameInternationalization
  • Games
  • General
  • Groups
  • Inventory
  • ItemConfiguration
  • Locale
  • Metrics
  • Notifications
  • Other
  • PremiumFeatures
  • Presence
  • PrivateMessages
  • Publish
  • Thumbnails
  • TranslationRoles
  • TwoStepVerification
  • Users

Critical Changes

  • CaptchaAPI was removed
  • AdsAPI was removed
  • TradesAPI was removed

@Syrilai Syrilai self-assigned this Jan 7, 2022
@Syrilai Syrilai linked an issue Jan 7, 2022 that may be closed by this pull request
package.json Outdated Show resolved Hide resolved
@guidojw
Copy link
Contributor

guidojw commented Jan 8, 2022

Commented everything I done on my fork (+ #279 & #280)

@guidojw
Copy link
Contributor

guidojw commented Jan 8, 2022

Is there a reason why you didn't choose to use export * from './path' syntax in index.ts?

connect (): Promise<void> {
return new Promise((resolve, reject) => {
const connectSocket = (retries = 0): void => {
this.socket = new SignalR.client("wss:https://realtime.roblox.com/notifications", ["usernotificationhub"], 3, true);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider replacing usage of signalr-client (deprecated) with https://www.npmjs.com/package/signalr

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you perhaps know if Roblox uses ASP.NET or ASP.NET Core? Since Microsoft offers a signalr client as well (https://www.npmjs.com/package/@microsoft/signalr) which targets ASP.NET Core (and I'm not entirely sure if it also works with ASP.NET SignalR servers)

Copy link

@jaydns jaydns Jan 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you perhaps know if Roblox uses ASP.NET or ASP.NET Core? Since Microsoft offers a signalr client as well (npmjs.com/package/@microsoft/signalr) which targets ASP.NET Core (and I'm not entirely sure if it also works with ASP.NET SignalR servers)

Fairly certain @microsoft/signalr only works with ASP.NET Core SignalR servers, and signalr only works with Normal ASP.NET SignalR servers. Both signalr and @microsoft/signalr are published by the same person on NPM -- wouldn't make much sense to publish a redundant package.

Roblox uses ASP.NET on realtime.roblox.com, judging by the headers. So signalr is the proper package to use.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Roblox uses both SignalR for .NET Core and .NET Framework. But for the likes of Messaging Service (Message Router Service) and Real-Time notifications, which are both part of the BEDEV1 Framework (Primarily .NET Framework, becoming obsolete), they use Microsoft’s Owin integration of SignalR

@ghost

This comment has been minimized.

@Syrilai
Copy link
Collaborator Author

Syrilai commented Jan 8, 2022

Commented everything I done on my fork (+ #279 & #280)

@guidojw Guessing because I worked the entire night, and I did the imports at around 7 am. Should be changed with the next commit that I have on hand, just need to update all typings in the apis first.

@Syrilai Syrilai added this to the v6 milestone Jan 9, 2022
@Syrilai Syrilai added the v6 label Jan 9, 2022
@jaydns
Copy link

jaydns commented Jan 20, 2022

compiled typescript output should stay in gitignore since it's not source files and can just be recompiled

@Syrilai
Copy link
Collaborator Author

Syrilai commented Jan 20, 2022

compiled typescript output should stay in gitignore since it's not source files and can just be recompiled

This allows to install the branch with npm and will only stay for a bit

@guidojw
Copy link
Contributor

guidojw commented Feb 27, 2022

After some digging, I finally found the cause of the error where X-CSRF token refreshing doesn't always correctly work:

The status message handler lowercases the received status message before comparing it to the set allowed/disallowed status messages. The set token validation error status message includes uppercase letters so this fails: https://github.com/Visualizememe/bloxy/blob/ea5281d6e99013bec5a57cc886bb85fa5ea861c4/src/controllers/rest/request/prepare.ts#L50

A good fix would be lowercasing statusMessage here
https://github.com/Visualizememe/bloxy/blob/ea5281d6e99013bec5a57cc886bb85fa5ea861c4/src/controllers/rest/response/handlers/validStatusMessage.ts#L18 and here https://github.com/Visualizememe/bloxy/blob/ea5281d6e99013bec5a57cc886bb85fa5ea861c4/src/controllers/rest/response/handlers/validStatusMessage.ts#L22

Although not really relevant for this PR, it seems like a good fix to include here.

@guidojw
Copy link
Contributor

guidojw commented Feb 27, 2022

Also what's the timeline on this? I see the bloxyjs/core repository, is that gonna be the new home for this project? I read the linked issue, and seems like this is already on there 👍🏿

@Syrilai
Copy link
Collaborator Author

Syrilai commented Feb 27, 2022

Also what's the timeline on this? I see the bloxyjs/core repository, is that gonna be the new home for this project? I read the linked issue, and seems like this is already on there 👍🏿

Yeah, bloxyjs/core is the new repository, since Martini unfortunately still did not manage to transfer the repository

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Export types in index.ts
4 participants