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

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

Merged
merged 3 commits into from
Nov 11, 2020

Conversation

ashmaroli
Copy link
Member

Local tests show that the local preview server is based on config["host"] and config["port"] values..
So even if config["url"] == https://jekyllrb.com, the site will be served at the default setting: //127.0.0.1:4000

This simplifies users' workflow by not having to build the site again with JEKYLL_ENV=production
(unless their templates are designed to render differently for the two ENV vars, which is a different story altogether)

P.S. The above observation was made on Windows. Please confirm for Unix platforms, if possible.

Copy link
Member

@DirtyF DirtyF left a comment

Choose a reason for hiding this comment

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

As long as default env is development and that in this context we serve on localhost, no matter what is in the config, I'm 👍

@ashmaroli
Copy link
Member Author

@DirtyF Did you give this branch a test-run locally..?

@DirtyF
Copy link
Member

DirtyF commented Sep 18, 2018

@ashmaroli I tested my blog and jekyll's website locally and url equals site.url in feed.xml and robots.txt without having to specify production environment on the command line.

@ashmaroli ashmaroli added this to the 4.0 milestone Jun 26, 2019
@DirtyF DirtyF removed the request for review from Crunch09 August 1, 2019 20:22
@DirtyF DirtyF requested a review from mattr- August 1, 2019 20:22
@mattr- mattr- removed this from the 4.0 milestone Aug 3, 2019
@DirtyF
Copy link
Member

DirtyF commented Nov 11, 2020

@jekyll: merge +minor

@jekyllbot jekyllbot merged commit 91a3dd9 into jekyll:master Nov 11, 2020
jekyllbot added a commit that referenced this pull request Nov 11, 2020
@DirtyF DirtyF added this to the 4.2 milestone Nov 11, 2020
@DirtyF

This comment has been minimized.

@ashmaroli

This comment has been minimized.

@benlk
Copy link
Contributor

benlk commented Feb 10, 2021

If you, like me, have found this ticket while trying to debug absolute_url using the wrong URL under bundle exec jekyll serve, or you have discovered that upgrading to 4.2 broke your local development workflow and have been pointed to https://jekyllrb.com/news/2020/12/14/jekyll-4-2-0-released/ as an explanation that this was a planned change, you will be happy to know that there's a command-line workaround documented in #8541 (comment)

@DirtyF
Copy link
Member

DirtyF commented Feb 11, 2021

@ashmaroli looking back at this change, I don't think it was the right move. 🤷

@benlk
Copy link
Contributor

benlk commented Feb 11, 2021

If conversation is going to be reopened on whether or not this change was the right move, I'll add my two cents:

This simplifies users' workflow by not having to build the site again with JEKYLL_ENV=production
(unless their templates are designed to render differently for the two ENV vars, which is a different story altogether)

When running bundle exec jekyll serve, with asset URLs piped through the absolute_url filter, under Jekyll 4.2, my localhost development site served at http://localhost:4000/ attempts to load assets like images, stylesheets, scripts, fonts and so on via the production URL https://example.com/. If those assets exist on the production server, then they load.

However, this PR's change leads to problems in the following cases:

  • images not yet uploaded to the production server do not appear, instead failing with 404s
  • stylesheets on prod don't have changes that were made to the http://localhost:4000/ version of the file
  • scripts on prod don't match markup served from http://localhost:4000/
  • font files fail to load because the production server doesn't supply a CORS header that allows their use on http://localhost:4000/

To avoid the problem of deploying files with http://localhost:4000/ URLs to https://example.com/, my deploy script runs jekyll clean before jekyll build, and then uploads to the production server.

I often run bundle exec jekyll serve with --drafts --unpublished. Even if I changed all my asset URL handling to be relative URLs, I would still jekyll clean to avoid uploading draft or unpublished work to the production site.

This change doesn't help me, and it doesn't really harm me. All it does is change my default local development command from bundle exec jekyll serve to bundle exec jekyll serve --host localhost --port 4000 — it's not a terrible bother, but it took me three hours to find what had changed.


Whether or not this change is undone, https://jekyllrb.com/docs/configuration/options/#serve-command-options should be updated to:

  • describe the effects of serve upon the variables it treats differently
  • state that serve serves from the same build directory used by jekyll build, and encourage users to run jekyll build before deploying files from that directory to a production server.

@evanwill
Copy link

I also just don't quite understand the justification for the change.
I thought the purpose of using absolute_url or site.url in templates was that it would be automatically swapped out on jekyll s vs. jekyll build?
The change definitely breaks the majority of my older projects, so I will have to hold them all back at Jekyll 4.1.1 or explain to users that you need to add --port 4001 to revert to expected behavior.

As @benlk mentions, in either case it would be helpful to clarify the documentation.

@ChaelCodes
Copy link

Documentation should be updated when major changes like this occur. The variables documentation makes some promises about the behavior of site.url that are no longer true.

benlk added a commit to benlk/jekyll that referenced this pull request Mar 29, 2021
@benlk
Copy link
Contributor

benlk commented Mar 29, 2021

I have filed a PR to revert these changes: #8620

Sorry for the delay in getting them to you.

jekyllbot pushed 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
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"
benknoble added a commit to benknoble/benknoble.github.io that referenced this pull request Oct 31, 2021
Upgrading to jekyll 4.2.0 picked up a new feature which causes local
development builds to not function the same as before. More details:
jekyll/jekyll#8541,
jekyll/jekyll#7253
@jekyll jekyll locked and limited conversation to collaborators Mar 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants