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

Convert AsyncProcessingQueue to typescript #4098

Merged
merged 9 commits into from
Oct 24, 2022
Merged

Conversation

jonaylor89
Copy link
Contributor

@jonaylor89 jonaylor89 commented Oct 12, 2022

Description

This PR converts AsyncProcessingQueue to typescript and changes how its exported to align with the other typescript files

Tests

I'm using the original tests to make sure nothing broke

Monitoring - How will this change be monitored? Are there sufficient logs / alerts?

This will be monitored with the typechecker

@jonaylor89 jonaylor89 changed the title type asyncprocessingqueue Convert AsyncProcessingQueue to typescript Oct 12, 2022
@jonaylor89 jonaylor89 marked this pull request as ready for review October 12, 2022 18:12
Copy link
Contributor

@theoilie theoilie left a comment

Choose a reason for hiding this comment

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

nice work, just some nits

@jonaylor89 jonaylor89 merged commit 0620f43 into master Oct 24, 2022
@jonaylor89 jonaylor89 deleted the jn-type-asyncqueue branch October 24, 2022 18:21
Copy link
Contributor

@vicky-g vicky-g left a comment

Choose a reason for hiding this comment

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

looks generally good, some comments here / a little concerned about the logContext (see the comments). was this tested locally with a track upload?

creator-node/src/AsyncProcessingQueue.ts Show resolved Hide resolved
})
} else {
// eslint-disable-next-line @typescript-eslint/no-floating-promises
Copy link
Contributor

Choose a reason for hiding this comment

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

can we just have 1 global lint for this floating promise?

Copy link
Contributor

Choose a reason for hiding this comment

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

if so, let's remove the one liners and add it to the global top of the file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can ad a file-wide lint. I'll make a note to add it in the follow up PR with the CustomRequest type


const ASYNC_PROCESSING_QUEUE_HISTORY = 500
type AddTaskParams = {
logContext: { requestID: string }
Copy link
Contributor

Choose a reason for hiding this comment

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

i think we should make this type any because even though we're only printing the requestID here, there may be other fields that are passed in.

https://github.com/AudiusProject/audius-protocol/pull/4098/files#diff-330e68b54fe69419f61d4adb76b7b255968de4a8b28166059adc53b03fae1136R222

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanna avoid making it any if possible. Perhaps we can add a LogContext type in utils somewhere so both CustomRequest and AddTaskParams can use it

Copy link
Contributor

Choose a reason for hiding this comment

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

is there a reason you are against using the type any? i would support making LogContext a type but i think there are too many different additional variables besides the base fields (requestID, requestMethod, etc. see getRequestLoggingContext())

creator-node/src/AsyncProcessingQueue.ts Show resolved Hide resolved
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.

None yet

3 participants