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

Bug 1701578 - Remove more circular dependencies #184

Merged
merged 10 commits into from
Apr 12, 2021

Conversation

Dexterp37
Copy link
Contributor

This is a big PR, sorry. However, it's fairly well split across commits and takes down the detected circular dependencies from 17 to ....

We're almost there!

This removes the direct dependency on Glean.ts. There's
still an indirect dependency on that due to the metric types.
This removes a circular dependency and eases further
removal of the remaining circular dependencies.
This removes a fake circular dependency related to the
'uuid' package imported via NPM.
This makes sure Glean.ts isn't mistakenly imported, as
a circular dependency, when it's not strictly needed.
Importing as a type is available since TS 3.8 and allows
to import types that are exclusively used for annotations
and removed after the build.
This additionally changes the Madge configuration to not
complain when type imports are used. See pahen/madge#232
@Dexterp37 Dexterp37 requested a review from brizental April 12, 2021 05:43
@Dexterp37 Dexterp37 self-assigned this Apr 12, 2021
Copy link
Contributor

@brizental brizental left a comment

Choose a reason for hiding this comment

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

Hey, great work here. Glad we are almost done with the circular kittens :D

I do not understand the purpose of dcd9ef2. How does this change make sure Glean.ts isn't mistakenly imported?

glean/src/core/internal_metrics.ts Outdated Show resolved Hide resolved
glean/src/core/metrics/events_database.ts Show resolved Hide resolved
@Dexterp37
Copy link
Contributor Author

Hey, great work here. Glad we are almost done with the circular kittens :D

I do not understand the purpose of dcd9ef2. How does this change make sure Glean.ts isn't mistakenly imported?

For example, in places like this one, we import the file but are exclusively interested to the internal metric, not the type. This file is further included in many chains that end up in the Glean.ts include line.

@Dexterp37 Dexterp37 requested a review from brizental April 12, 2021 13:31
@brizental
Copy link
Contributor

For example, in places like this one, we import the file but are exclusively interested to the internal metric, not the type. This file is further included in many chains that end up in the Glean.ts include line.

So, basically, put as little exports as possible in any files that include a import Glean from "glean" statement?

@Dexterp37
Copy link
Contributor Author

For example, in places like this one, we import the file but are exclusively interested to the internal metric, not the type. This file is further included in many chains that end up in the Glean.ts include line.

So, basically, put as little exports as possible in any files that include a import Glean from "glean" statement?

By splitting we are actually guaranteeing that the e.g. 'datetime_metric.ts' file, included in many places, does not include Glean.ts at all: so utils.ts won't require Glean.ts at all!

@Dexterp37 Dexterp37 merged commit 4c18989 into mozilla:main Apr 12, 2021
@Dexterp37 Dexterp37 deleted the internal_metrics branch April 12, 2021 15:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants