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

refactor(lambdas): Upgrade eslint config to "recommended" and fix violations in code #21155

Merged
merged 20 commits into from
May 20, 2024

Conversation

Josmithr
Copy link
Contributor

@Josmithr Josmithr commented May 18, 2024

The minimal config is deprecated and scheduled for deletion. Updates the package to use the "recommended" config and fixes violations in code.

Disabled a number of rules in the config due to the sheer number of violations. These can (and should) be fixed at a later date.

AB#8002

@github-actions github-actions bot added area: server Server related issues (routerlicious) base: main PRs targeted against main branch labels May 18, 2024
@Josmithr Josmithr requested review from a team and znewton May 18, 2024 00:46
@Josmithr Josmithr marked this pull request as ready for review May 18, 2024 00:47
server/routerlicious/packages/lambdas/.eslintrc.cjs Outdated Show resolved Hide resolved
@@ -62,7 +62,7 @@ export class BroadcasterLambda implements IPartitionLambda {
private readonly clientManager: IClientManager | undefined,
) {}

public async handler(message: IQueuedMessage) {
public async handler(message: IQueuedMessage): Promise<undefined> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like this should be Promise<void> and remove the return?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, this seemed very strange. Initially I was leaning towards avoiding any disruption here, but this seems safe enough to clean up. Will do.

Copy link
Contributor Author

@Josmithr Josmithr May 20, 2024

Choose a reason for hiding this comment

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

Actually, this does seem to be intentional, based on usage patterns. I've updated the docs in IPartitionLambda to call out what's going on, and annotated the implementations with @inheritDoc comments to help signal that the specialness is documented.

This pattern seems odd to me, and could probably be improved, but it does appear to have been intentional and is not as simple as simply changing the return types to void.

Edit: the general pattern is intentional. This specific return type was incorrect. It is async, so it should have been Promise<void>. This has been resolved here.

Comment on lines +451 to +455
} else if (
value.operation.type === MessageType.ClientJoin &&
this.localCheckpointEnabled
) {
this.globalCheckpointOnly = false;
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 I'd revert this and disable the lint rule locally to keep the if-else structure just with the value.operation.type checks.

server/routerlicious/packages/lambdas/src/deli/lambda.ts Outdated Show resolved Hide resolved
@Josmithr Josmithr requested review from alexvy86 and a team May 20, 2024 18:15
@Josmithr Josmithr enabled auto-merge (squash) May 20, 2024 18:25
@Josmithr Josmithr merged commit 7084c23 into microsoft:main May 20, 2024
29 checks passed
@Josmithr Josmithr deleted the lambdas/upgrade-eslint-config branch May 20, 2024 18:30
kekachmar pushed a commit to kekachmar/FluidFramework that referenced this pull request May 21, 2024
…lations in code (microsoft#21155)

The minimal config is deprecated and scheduled for deletion. Updates the
package to use the "recommended" config and fixes violations in code.

Disabled a number of rules in the config due to the sheer number of
violations. These can (and should) be fixed at a later date.

[AB#8002](https://dev.azure.com/fluidframework/235294da-091d-4c29-84fc-cdfc3d90890b/_workitems/edit/8002)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: server Server related issues (routerlicious) base: main PRs targeted against main branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants