Skip to content

Conversation

@benface
Copy link
Contributor

@benface benface commented Mar 16, 2023

Some proposed changes to accompany my review of #301...

@benface benface requested a review from a team as a code owner March 16, 2023 22:18
@github-actions
Copy link

github-actions bot commented Mar 16, 2023

📦 Next.js Bundle Analysis

This analysis was generated by the next.js bundle analysis action 🤖

⚠️ Global Bundle Size Increased

Page Size (compressed)
global 1.39 MB (🟡 +16 B)
Details

The global bundle is the javascript bundle that loads alongside every page. It is in its own category because its impact is much higher - an increase to its size means that every page on your website loads slower, and a decrease means every page loads faster.

Any third party scripts you have added directly to your app using the <script> tag are not accounted for in this analysis

If you want further insight into what is behind the changes, give @next/bundle-analyzer a try!

One Page Changed Size

The following page changed size from the code in this PR compared to its base branch:

Page Size (compressed) First Load % of Budget (350 KB)
/[locale] 2.08 KB 1.39 MB 406.22% (+/- <0.01%)
Details

Only the gzipped size is provided here based on an expert tip.

First Load is the size of the global bundle plus the bundle for the individual page. If a user were to show up to your website and land on a given page, the first load size represents the amount of javascript that user would need to download. If next/link is used, subsequent page loads would only need to download that page's bundle (the number in the "Size" column), since the global bundle has already been downloaded.

Any third party scripts you have added directly to your app using the <script> tag are not accounted for in this analysis

The "Budget %" column shows what percentage of your performance budget the First Load total takes up. For example, if your budget was 100kb, and a given page's first load size was 10kb, it would be 10% of your budget. You can also see how much this has increased or decreased compared to the base branch of your PR. If this percentage has increased by 20% or more, there will be a red status indicator applied, indicating that special attention should be given to this. If you see "+/- <0.01%" it means that there was a change in bundle size, but it is a trivial enough amount that it can be ignored.

@benface benface mentioned this pull request Mar 16, 2023
@saihaj
Copy link
Contributor

saihaj commented Mar 17, 2023

@benface if you can rebase from nextra branch and just have your change commits cause right now the diff is noise cause of the changes from main which were not synced yet

@benface
Copy link
Contributor Author

benface commented Mar 17, 2023

@saihaj – I had intentionally merged main into my branch, but I guess that's problematic, especially with squashing commits. I just did like you said. Better now?

@saihaj
Copy link
Contributor

saihaj commented Mar 17, 2023

@saihaj – I had intentionally merged main into my branch, but I guess that's problematic, especially with squashing commits. I just did like you said. Better now?

yeah now we can easily go through commits!

Comment on lines -24 to +25
href={`https://github.com/graphprotocol/docs/blob/main/pages/${pagePath}`}
href={`https://github.com/graphprotocol/docs/blob/main/website/${pagePath}`}
Copy link
Contributor

Choose a reason for hiding this comment

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

nice catch for edit on github, forget to test it

"compilerOptions": {
"baseUrl": ".",
"paths": {
"@/*": ["*"]
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 this was nice :)

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 agree, if you find a way to make it pass pnpm lint, let's put it back!

Comment on lines 24 to 25
import { getNavItems } from '@/navigation'
import { getNavItems } from '${'../'.repeat(routeSegments.length - 1)}navigation'
Copy link
Contributor

Choose a reason for hiding this comment

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

why did you remove @ alias in tsconfig? seems it worked well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It didn't! Try running pnpm lint in your branch.

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 ESlint config has a way to pick aliases 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yeah Prettier fails which prevents tsc from even running... try pnpm lint:fix

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 should probably replace the &&s with ; so each command runs regardless of the result of the previous one.

Copy link
Contributor

Choose a reason for hiding this comment

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

to fix you can replace the below in package.json

-"typecheck": "tsc --noEmit",
+"typecheck": "pnpm --filter @graphprotocol/docs typecheck",

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ohhh, thank you @B2o5T! Done in a0717fa, and then I reverted removing the alias in e8baaf6.

By the way, there are still 4 ESLint warnings:

CleanShot 2023-03-16 at 22 20 47@2x

Those are weird, I don't really understand why TypeScript thinks they are any.

Copy link
Contributor

Choose a reason for hiding this comment

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

I will take a look tomorrow!

@saihaj saihaj mentioned this pull request Mar 17, 2023
package.json Outdated
Comment on lines 10 to 11
"lint": "eslint . --ext .js,.jsx,.ts,.tsx,.mjs; pnpm prettier:check; pnpm typecheck",
"lint:fix": "eslint . --fix --ext .js,.jsx,.ts,.tsx,.mjs; pnpm prettier; pnpm typecheck",
Copy link
Contributor

Choose a reason for hiding this comment

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

@benface we should use && instead, otherwise, it's not fails if some command fails, e.g. eslint --max-warnings 0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yeah, oops. I guess I just find it annoying that if e.g. Prettier fails, the TS errors are not even shown. Maybe Prettier should be last?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, I guess the following commands typecheck -> eslint -> prettier will be better

@dimaMachina dimaMachina merged commit 3c9d8cc into nextra Mar 17, 2023
@dimaMachina dimaMachina deleted the benface/nextra branch March 17, 2023 20:40
dimaMachina added a commit that referenced this pull request Mar 17, 2023
Co-authored-by: Dimitri POSTOLOV <[email protected]>
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.

4 participants