refactor: rename img-url to media-url and site.img_cdn to site.cdn

[MrMurdock11]

Type of change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Improvement (refactoring and improving code)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Documentation update

Description

We discussed the variable changes in this PR.

[cotes2020]

site.cdn is not just for images, so the documentation and code comments have to be changed accordingly.

Also I think the parameter of _includes/media-url.html should be changed from img_path to subpath.

[kungfux]

Also I think the parameter of _includes/media-url.html should be changed from img_path to subpath.

I'm not sure if it worst to rename parameters here as I would like to propose to refactor configuration to improve the support of image/video/audio embed. Just so you know. Here is a draft idea:

# Media files configuration
media:
  # Use CDN for media files e.g. "https://cdn.example.com"
  # If defined, CDN URL will be added to all image (site avatar & posts images) paths starting with '/'
  cdn: "https://chirpy-img.netlify.app"
  # Default path bases for images/videos/audios e.g. "/assets/images"
  # Can be extended by img_path/video_path/audio_path variables from post front matter
  # like site.base_paths.img_path + post.img_path
  base_path:
    img: "/assets/images"
    video: &video_base_path "/assets/video"
    audio: &audio_base_path "/assets/audio"

pwa:
  cache:
    enabled: true
    deny_paths:
      - *video_base_path
      - *audio_base_path

[cotes2020]

@kungfux Even if you add media.base_path to _config.yml, it still doesn't replace the existing page.xxx_path functionality.

So if you want to extend subpath support for video, you can just add page.video_path, and the same goes for audio.

[kungfux]

@cotes2020 That's correct and I wish to have it. The base path just makes it handy to omit specifying the full path every time.

So, the question is: do we want to accomplish some renaming here first or work on the whole solution right away instead. I have shared the idea just to avoid repeating work on the same things. Thanks.

[cotes2020]

... do we want to accomplish some renaming here first or work on the whole solution right away instead.

I think it's fine. Let's see what @MrMurdock11 thinks.

[MrMurdock11]

@cotes2020 I agree with @kungfux. I think we can do this now. My preparation process with renaming and new parameters for @kungfux's config is easier to release as a PR with a detailed description of what it's all for

[cotes2020]

Do you mean that the following configuration is feasible? @MrMurdock11

# Media files configuration
media:
  # ...
  # Default path bases for images/videos/audios e.g. "/assets/images"
  # Can be extended by img_path/video_path/audio_path variables from post front matter
  # like site.base_paths.img_path + post.img_path
  base_path:
    img: "/assets/images"
    video: &video_base_path "/assets/video"
    audio: &audio_base_path "/assets/audio"

# ...

[MrMurdock11]

@cotes2020 If you mean anchors in yaml, that's a hypothesis I really want to test, I like it. If it is feasible, the file configuration may simplify in some cases.

Based on the documentation, we can assume that it will work. But this needs to be tested directly in the project.
P.S.: This functionality supports from YAML v1.0

[cotes2020]

Hi @MrMurdock11 Mr, I'm not discussing anchors in yaml, but I'm worried that you're going to add a structure like media.base_path.{img|video|audio} to _config.yml, because it's unnecessary as it doesn't replace the functionality of the existing page.img_path (front-matter in the header of the post) and adds complexity to the configuration.

[MrMurdock11]

@cotes2020 After discussing @kungfux's suggestion offline, I realized that the root of his solution is a bug with caching large amounts of audio/video files. After analyzing the code base, I came to the conclusion that you are right and there is no point in complicating the configuration file in this case, because we can add code to avoid this bug in the future. Below is the code that should fix this.

// ./sw.js

- if (purge || event.request.method !== 'GET' || !verifyUrl(url)) {
+ if (purge || response.status !== 200 || event.request.method !== 'GET' || !verifyUrl(url)) {
  return response;
}

Based on this, I'll make the corrections you mentioned at the beginning and create a separate PR to fix this problem.

[MrMurdock11]

site.cdn is not just for images, so the documentation and code comments have to be changed accordingly.

Also I think the parameter of _includes/media-url.html should be changed from img_path to subpath.

What do u think do we need to rename img_path: '/posts/20180809' to media_(sub)path: '/posts/20180809' for page scope?

[cotes2020]

What do u think do we need to rename img_path: '/posts/20180809' to media_(sub)path: '/posts/20180809' for page scope?

OK, I think both names are fine (page.media_path / page.media_subpath). It's your call. :)

[MrMurdock11]

@cotes2020 @kungfux I've pushed my changes. If you have time, please check

[cotes2020]

LGTM

[cotes2020]

Since this is a BREAKING CHANGE, I'll fix the commit message with force-push.