Bug 1701578 - Break the circular dependency between pings/index.ts and pings/maker.ts #180
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Dexterp37
force-pushed
the
ping_maker_dep
branch
from
e86926c to
b3c5dcb
Compare
brizental
reviewed
Apr 9, 2021
We can tell the TS compiler, when declaring the variable, to expect it to be initialized shortly after construction.
This breaks yet another circular dependency on PingType.
This uses "@typescript-eslint/semi" instead of just "semi" as recommended https://github.com/typescript-eslint/typescript-eslint/blob/master/docs/getting-started/linting/FAQ.md#i-am-using-a-rule-from-eslint-core-and-it-doesnt-work-correctly-with-typescript-code
Dexterp37
force-pushed
the
ping_maker_dep
branch
from
b3c5dcb to
7b674b1
Compare
Dexterp37
changed the title
brizental
approved these changes
Apr 9, 2021
| // The value of the X-Source-Tags header to be included in every ping. | ||
| sourceTags?: string[], | ||
| } | ||
| import { DebugOptions } from "./debug_options.js"; |
Contributor
There was a problem hiding this comment.
Why was it necessary to move this into another file?
Contributor
Author
There was a problem hiding this comment.
Because due to the other imports in the config.ts file, if we wanted to use DebugOptions in the pings module without splitting it, we'd create a new circular dependency :)
This change is why this PR allows us to drop 2 circular dependencies :)
| */ | ||
| export async function collectAndStorePing(identifier: string, ping: PingType, reason?: string): Promise<void> { | ||
| const collectedPayload = await collectPing(ping, reason); | ||
| export async function collectAndStorePing(metricsDatabase: MetricsDatabase, eventsDatabase: EventsDatabase, pingsDatabase: PingsDatabase, applicationId: string, identifier: string, ping: CommonPingData, reason?: string, debugOptions?: DebugOptions): Promise<void> { |
Contributor
There was a problem hiding this comment.
nit: Give a default value to the debug options and we don't need to put in the end of the arguments list.
Contributor
Author
There was a problem hiding this comment.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
With this, we're down to
1817! 🥳I also fixed a bug in our linting configuration about semicolon!