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

[Nextjs] Allow empty string for getPublicUrl #1656

Merged
merged 15 commits into from
Nov 23, 2023
Merged

[Nextjs] Allow empty string for getPublicUrl #1656

merged 15 commits into from
Nov 23, 2023

Conversation

yavorsk
Copy link
Contributor

@yavorsk yavorsk commented Nov 7, 2023

Description / Motivation

In production non editing environments, it is desirable to use relative URLs for static assets (e.g. _next/*, images, etc). For this we need to allow for the PUBLIC_URL environment variable to be set as empty string - comment the PUBLIC_URL in the default .env; adjust the logic in the getPublicUrl() function so that empty string is not treated as wrong and also takes precedence over vercel and netlify variables.
Connected with that this PR addresses a specific scenario where in editing environment a component makes a call to a nextjs app api route, which fails because we don't have access to PUBLIC_URL on the client side.

Testing Details

  • Unit Test Added
  • Manual Test/Other (Please elaborate)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

@yavorsk yavorsk changed the title Feature/jss 747 [Nextjs] Allow empty string for getPublicUrl Nov 7, 2023
Copy link
Contributor

@ambrauer ambrauer left a comment

Choose a reason for hiding this comment

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

Looking good, see a couple comments

Copy link
Contributor

@ambrauer ambrauer left a comment

Choose a reason for hiding this comment

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

Bit of scope increase, but now that we have the publicUrl available on the JSS config, I think we should switch over to use it exclusively in the app template (vs calling getPublicUrl() everywhere).

Here are the locations:
https://github.com/search?q=repo%3ASitecore%2Fjss+getPublicUrl%28%29+path%3A%2F%5Epackages%5C%2Fcreate-sitecore-jss%5C%2Fsrc%5C%2Ftemplates%5C%2F%2F&type=code

CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Contributor

@ambrauer ambrauer left a comment

Choose a reason for hiding this comment

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

One minor language fix if you want to change those, otherwise looks great!

@@ -58,7 +57,7 @@ const sitemapApi = async (
const lastSegment = parseUrl[parseUrl.length - 1];

return `<sitemap>
<loc>${getPublicUrl()}/${lastSegment}</loc>
<loc>${config.publicUrl}/${lastSegment}</loc>
Copy link
Contributor

@illiakovalenko illiakovalenko Nov 16, 2023

Choose a reason for hiding this comment

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

@yavorsk Can you, please, verify that sitemap api still works as expected?
Potentially, as @ambrauer mentioned, an empty string can't be used here (so the sitemap will not be working), so it should be tested.

Copy link

Choose a reason for hiding this comment

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

@matkovskyi can you verify if using empty string breaks your functionality?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@kendoce Yes, an empty string can't be used here

@yavorsk yavorsk requested a review from ambrauer November 22, 2023 15:01
@@ -50,14 +50,16 @@ const sitemapApi = async (
if (!sitemaps.length) {
return res.redirect('/404');
}


const reqtHost = req.headers.host;
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this handle localhost?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep, tested

Copy link
Contributor

@ambrauer ambrauer left a comment

Choose a reason for hiding this comment

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

Looks good, assuming localhost is handled (see comment)

Copy link
Contributor

@illiakovalenko illiakovalenko left a comment

Choose a reason for hiding this comment

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

👍

@yavorsk yavorsk merged commit 7b26ef9 into dev Nov 23, 2023
1 check passed
@yavorsk yavorsk deleted the feature/JSS-747 branch November 23, 2023 14:10
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.

5 participants