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

Revert #7253: "Don't reset site.url to localhost:4000 by default" #8620

Merged
merged 8 commits into from Sep 15, 2021

Conversation

benlk
Copy link
Contributor

@benlk benlk commented Mar 29, 2021

This is a 🐛 bug fix.
I have not run the CI suite.

Summary

When running with bundle exec jekyll serve and no other options, the absolute_url filter will now use the local server URL instead of the URL from the config (like benlk.com) when that URL is the default localhost:4000.

Context

This PR is designed to fix the regression introduced in #7253 and described in my comments there: #7253 (comment)

If you don't want to check the links, here's a summary:

  • Don't reset site.url to localhost:4000 by default #7253 changed the behavior of jekyll serve to use the site URL from the site's config when serving a local development version of the site, if the localhost host and port were not explicitly specified. This meant that site.url and the absolute_url filter would use the production URL when generating and serving the site locally. The documentation on jekyllrb.org was not updated to reflect this change in behavior, so people who innocently ran jekyll serve and expected assets to load from their local development server would instead see:
    • images loaded from the production server, or 404 if not present on the production server
    • stylesheets loaded from the production server without changes from edits made locally, or 404 if not present on the production server
    • scripts loaded from the production server without changes from edits made locally, or 404 if not present on the production server
    • font files failing to load from production servers not configured to allow CORS requests for font files from localhost:4000, or 404 if not present on the production server
  • That change did not affect cases where the local server URL was explicitly set via bundle exec jekyll serve --host localhost --port 4000 to be localhost:4000, or when it was set to be any other combination of hosts or ports. That change only affected the default jekyll serve, which is the local development action recommended in the Jekyll docs.

This PR does update documentation with the recommendations from discussion in #7253

Reverts #7253

@benlk
Copy link
Contributor Author

benlk commented Mar 30, 2021

Before merging, this PR should also update the usage docs https://github.com/jekyll/jekyll/edit/master/docs/_docs/usage.md to describe the intended use of build and serve in their default configurations, and how to override those configurations to account for the use that #7253 wanted to satisfy.

@DirtyF DirtyF requested review from a team, parkr and ashmaroli and removed request for a team May 14, 2021 19:12
@mattr-
Copy link
Member

mattr- commented May 16, 2021

Before merging, this PR should also update the usage docs https://github.com/jekyll/jekyll/edit/master/docs/_docs/usage.md to describe the intended use of build and serve in their default configurations, and how to override those configurations to account for the use that #7253 wanted to satisfy.

I agree with this comment. Is this something you'll do soon? It doesn't seem productive to do anything else with this PR if it's not complete.

@benlk
Copy link
Contributor Author

benlk commented May 25, 2021

@mattr- It's something that I'd prefer to leave to a maintainer, but I can draft some language and add it to the PR if you'd like.

@mattr-
Copy link
Member

mattr- commented May 25, 2021

@mattr- It's something that I'd prefer to leave to a maintainer, but I can draft some language and add it to the PR if you'd like.

I would appreciate it if you would draft some language and include it in the PR. Thanks! ❤️

@benlk
Copy link
Contributor Author

benlk commented Jun 4, 2021

@mattr- I've updated the docs in this PR to better differentiate between build and serve; I'm open to revisions in the language: b298914

However, this doesn't address the latter part:

and how to override those configurations to account for the use that #7253 wanted to satisfy.

Is the current admonition to see the full list of commands and build options okay for that purpose?

For a full list of options and their argument, see Build Command Options.

docs/_docs/usage.md Outdated Show resolved Hide resolved
@ashmaroli ashmaroli changed the title Revert https://github.com/jekyll/jekyll/pull/7253 Revert #7253: "Don't reset site.url to localhost:4000 by default" Sep 15, 2021
@ashmaroli ashmaroli added the backport-candidate Consider for merge into an older stable branch label Sep 15, 2021
@mattr-
Copy link
Member

mattr- commented Sep 15, 2021

@jekyllbot: merge +fix

@jekyllbot jekyllbot merged commit faef38b into jekyll:master Sep 15, 2021
jekyllbot added a commit that referenced this pull request Sep 15, 2021
github-actions bot pushed a commit that referenced this pull request Sep 15, 2021
Ben Keith: Revert #7253: "Don't reset site.url to localhost:4000 by default" (#8620)

Merge pull request 8620
ashmaroli pushed a commit to ashmaroli/jekyll that referenced this pull request Sep 15, 2021
ashmaroli pushed a commit to ashmaroli/jekyll that referenced this pull request Sep 15, 2021
Revert jekyll#7253: "Don't reset site.url to localhost:4000 by default"
This backports faef38b to 4.2-stable
ashmaroli added a commit that referenced this pull request Sep 16, 2021
Backport #8620 for v4.2.x: Revert #7253: "Don't reset site.url to localhost:4000 by default"
@jekyll jekyll locked and limited conversation to collaborators Sep 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backport-candidate Consider for merge into an older stable branch bug fix frozen-due-to-age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants