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

ref(insights): Declare module base URLs as constants #70861

Merged
merged 4 commits into from
May 15, 2024

Conversation

gggritso
Copy link
Member

More preparation for moving all insights to the /insights/ URL namespace.

Now that every module is wrapped in ModulePageProviders I can set the correct baseURL for each one. BASE_URL is the base URL of the whole module rather than of the page. It's also not the full base URL, only the module itself, relevant to whatever comes before it (usually "/performance"), which is why they don't have starting slashes (except for AI).

Most importantly, almost nothing in the app actually uses baseURL to construct URLs, so this PR has almost no impact. The only affected links:

  • resource table
  • queries table
  • cache samples table

@gggritso gggritso requested a review from a team as a code owner May 14, 2024 15:53
@github-actions github-actions bot added the Scope: Frontend Automatically applied to PRs that change frontend components label May 14, 2024
Copy link

codecov bot commented May 14, 2024

Bundle Report

Changes will decrease total bundle size by 52.14kB ⬇️

Bundle name Size Change
app-webpack-bundle-array-push 26.75MB 52.14kB ⬇️

@@ -178,7 +179,7 @@ function PageWithProviders({params}: Props) {
return (
<ModulePageProviders
title={[spanDescription ?? t('(no name)'), t('Pipeline Details')].join(' — ')}
baseURL="/ai-monitoring/"
baseURL={BASE_URL}
Copy link
Contributor

Choose a reason for hiding this comment

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

This also includes sentry locale package which you don't need in this file. This could easily come to a point where it effects build time and makes it harder to eliminate dead code. I'm against this pattern.

Copy link
Member Author

Choose a reason for hiding this comment

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

🤔 I see your point, but:

  1. In this PR, every file that imports BASE_URL already imports t, so I'm not introducing a new import
  2. Colocating regular constants and translated strings is a common pattern

If you feel strongly that this pattern is bad, I would love to see proof that it has a meaningful impact on build time, an argument that it's worth sacrificing colocation, and some suggestions on what to do instead

I think the FE TSC or the frontend Slack channel are a more appropriate venue for this, since it needs some discussion. If we all agree, we can introduce this to our developer guidelines and work on improving the situation

In the meantime, is this worth blocking this PR for, or is it a nit? If it's the latter, blocking the PR is probably not appropriate, unless I'm misinterpreting our PR reviewer guidelines

@gggritso gggritso enabled auto-merge (squash) May 15, 2024 12:59
@gggritso gggritso merged commit 70c361f into master May 15, 2024
42 checks passed
@gggritso gggritso deleted the ref/perf/module-base-urls branch May 15, 2024 13:12
@github-actions github-actions bot locked and limited conversation to collaborators May 31, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Scope: Frontend Automatically applied to PRs that change frontend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants