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

fix: sitemap url for production deployments #5599

Merged
merged 3 commits into from
Aug 2, 2023
Merged

Conversation

styfle
Copy link
Member

@styfle styfle commented Aug 1, 2023

Description

The sitemap for production deployments point to the wrong url.

You can see this by visiting https://nodejs.org/sitemap.xml

image

Validation

You'll need to set the NEXT_PUBLIC_BASE_URL env var and then visit /sitemap.xml to see the impact.

You can set the env var in the Vercel dashboard and redeploy, or you can try it locally via next dev by creating .env.local.

Related Issues

https://twitter.com/mayankistyping/status/1686288768326914048

Check List

  • I have read the Contributing Guidelines and made commit messages that follow the guideline.
  • I have run npx turbo lint to ensure the code follows the style guide. And run npx turbo lint:fix to fix the style errors if necessary.
  • I have run npx turbo format to ensure the code follows the style guide.
  • I have run npx turbo test to check if all tests are passing, and/or npx turbo test:snapshot to update snapshots if I created and/or updated React Components.
  • I've covered new added functionality with unit tests if necessary.

@styfle styfle requested a review from a team as a code owner August 1, 2023 18:54
@vercel
Copy link

vercel bot commented Aug 1, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
nodejs-org ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 1, 2023 10:17pm

@github-actions
Copy link

github-actions bot commented Aug 1, 2023

📦 Next.js Bundle Analysis for nodejs.org

This analysis was generated by the Next.js Bundle Analysis action. 🤖

⚠️ Global Bundle Size Increased

Page Size (compressed)
global 140.88 KB (6 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!

Two Pages Changed Size

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

Page Size (compressed) First Load
/404 34.41 KB (🟢 -22 B) 175.28 KB
/[...path] 34.34 KB (🟢 -22 B) 175.21 KB
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

Next to the size is how much the size has increased or decreased compared with the base branch of this PR. If this percentage has increased by undefined% or more, there will be a red status indicator applied, indicating that special attention should be given to this.

@github-actions
Copy link

github-actions bot commented Aug 1, 2023

Unit Test Coverage Report

Lines Statements Branches Functions
Coverage: 93%
92.42% (366/396) 79% (79/100) 88.09% (74/84)

Unit Test Report

Tests Skipped Failures Errors Time
45 0 💤 0 ❌ 0 🔥 8.078s ⏱️

Copy link
Member

@ovflowd ovflowd left a comment

Choose a reason for hiding this comment

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

I appreciate the effort, but I'm not keen on the approach 👀

next.constants.mjs Outdated Show resolved Hide resolved
Copy link
Member

@ovflowd ovflowd left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Collaborator

@bmuenzenmeyer bmuenzenmeyer left a comment

Choose a reason for hiding this comment

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

works for me, and FANTASTIC validation instructions. This was the exact thing I was hoping to unlock by introducing that.

@bmuenzenmeyer
Copy link
Collaborator

I agree that this appears to be a production bug - but not knowing the status of the env var or having access to vercel, I'd rather @ovflowd merge it in the morning

@ovflowd
Copy link
Member

ovflowd commented Aug 2, 2023

Yup that's right! Deploying now :)

PS: I didn't merge back then because I was already going to sleep!

@ovflowd ovflowd merged commit c36666c into nodejs:main Aug 2, 2023
@styfle styfle deleted the patch-1 branch August 2, 2023 12:22
@styfle
Copy link
Member Author

styfle commented Aug 2, 2023

@ovflowd Did you set the env var yet? I’m still seeing the bug on the latest deployment.

@ovflowd
Copy link
Member

ovflowd commented Aug 2, 2023

Of course I dit. And it was working when I made the deployment. Now when I open this (https://nodejs.org/sitemap.xml), it seems gone.

Is Vercel applying my Env Variables to the Serverless Functions? I have no idea why it would work then stop working.

@ovflowd
Copy link
Member

ovflowd commented Aug 2, 2023

image

@ovflowd
Copy link
Member

ovflowd commented Aug 2, 2023

Triggering a redeployment. What is even weirder is that the URL on the current sitemap, is coming from a Preview Deployment? (nodejs-lp1va9y4q)

image

@styfle
Copy link
Member Author

styfle commented Aug 2, 2023

What is even weirder is that the URL on the current sitemap, is coming from a Preview Deployment?

That sounds like its the issue. If you promoted a Preview deployment, then it wouldn't get Production env vars.

@ovflowd
Copy link
Member

ovflowd commented Aug 2, 2023

That sounds like its the issue. If you promoted a Preview deployment, then it wouldn't get Production env vars.

To clarify, we never promoted any "preview" build to production. We use default settings. Only "main" branch should go to "production". Is this by any chance a bug on Vercel-side?

@ovflowd
Copy link
Member

ovflowd commented Aug 2, 2023

It's fixed now, but pretty much because I did a "rebuild" of a production build. And this seems to be a thing that is recurring (from time to time these .vercel.app URLs from preview deployments seem to somehow get there)

Could aos by any chance Turborepo Cache be influencing here?

@styfle
Copy link
Member Author

styfle commented Aug 2, 2023

Ah yes it could be turborepo cache.

The deployment from PR #5595 would be considered Preview and then once merged, since there were no code changes, you might see "FULL TURBO" in the Production build logs meaning it would just reuse the built output from the PR.

You might want to add all the env vars you depend on to the turbo.json like this: https://turbo.build/repo/docs/core-concepts/caching/environment-variable-inputs

I'll ask the turborepo team if they have any better suggestions.

@ovflowd
Copy link
Member

ovflowd commented Aug 2, 2023

This is definitely relevant knowledge, right there! (Thank you @styfle !!)

@ovflowd
Copy link
Member

ovflowd commented Aug 2, 2023

Hot-fix: 97821bc

@styfle
Copy link
Member Author

styfle commented Aug 3, 2023

@ovflowd I spoke to the turborepo team and they said this is the expected behavior because default is "loose" env var handling. Adding the env vars like you did is a good solution 👍

If you wanted to catch this type of bug in the future, you could enable "strict" env var handling, which would prevent any env var from being used without explicitly marking it as part of the cache key hash or marking it as a pass-through.

https://turbo.build/repo/docs/core-concepts/caching/environment-variable-inputs#loose--strict-environment-modes

@ovflowd
Copy link
Member

ovflowd commented Aug 3, 2023

Thanks @styfle! Would you be kind enough and make a PR for strict env? 🙇

@styfle
Copy link
Member Author

styfle commented Aug 4, 2023

I have't tried it yet, so I'll have to update my own repos before this one 😆

@ovflowd
Copy link
Member

ovflowd commented Aug 4, 2023

Np! If you prefer I can handle the PR.

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.

3 participants