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

feat: 'optional_updated' option #3887

Closed
wants to merge 1 commit into from

Conversation

curbengh
Copy link
Contributor

@curbengh curbengh commented Nov 28, 2019

What does it do?

Continuation of #3235, specifically #3235 (comment).

User may prefer to have full control on value of post.updated and not use file modification time. Currently for my blog, I have to use a new variable lastUpdated as a workaround; since that variable is not recognized by many plugins, I have to fork many plugins.

With this PR, config.optional_updated can be enabled (disabled by default), to leave post.updated to be empty if updated: is not set in the front-matter.

Breaking changes

Post and Page model no longer assign default moment() value to updated property.

  • This can break plugin's and theme's unit test, regardless of optional_updated (because the option is not passed to the model), assuming all the following conditions exist:
    1. Uses Post/Page model (example)
    2. Uses post.updated/page.updated variable (example)
    3. Doesn't perform null-checking on post.updated/page.updated.
  • Outside of unit test (or in real world), this only break plugin and theme if optional_updated is enabled, assuming all the following conditions exist:
    1. Uses post.updated/page.updated variable
    2. Doesn't perform null-checking on post.updated/page.updated. (e.g. hexo-theme-next doesn't check)

How to test

git clone -b custom-updated https://github.com/curbengh/hexo.git
cd hexo
npm install
npm test

Screenshots

Pull request tasks

  • Add test cases for the changes.
  • Passed the CI test.

@coveralls
Copy link

coveralls commented Nov 28, 2019

Coverage Status

Coverage increased (+0.003%) to 97.125% when pulling 26133ca on curbengh:custom-updated into 6216d05 on hexojs:master.

@curbengh
Copy link
Contributor Author

curbengh commented Nov 28, 2019

@tomap Relating to the issues addressed by #3235, I prefer the approach of leaving post.updated to be empty.

The main reason is that it's easier for theme/plugin to handle, e.g.

if (post.updated && post.updated.toString() !== post.date.toString()) { %>

can be simplified to

if (post.updated) { %>

@@ -17,7 +17,6 @@ module.exports = ctx => {
},
updated: {
type: Moment,
default: moment,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is necessary to allow page.updated to be null.

data.comments.should.be.true;
data.layout.should.eql('post');
data._content.should.eql('');
data.link.should.eql('');
data.raw.should.eql('');
data.published.should.be.true;
should.not.exist(data.updated);
Copy link
Contributor Author

@curbengh curbengh Nov 28, 2019

Choose a reason for hiding this comment

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

In actual use, data.updated won't be null (if optional_updated is disabled, which is the default), thanks to

data.updated = stats.mtime;
.

@curbengh
Copy link
Contributor Author

curbengh commented Nov 28, 2019

Enabling this config will break themes/plugins that assume post.updated won't be null, especially the one which uses hexo as API in their unit test.

So far, I notice hexo-generator-sitemap is compatible, but not hexo-generator-feed.


I also tried to minimize the breaking change by retaining this line, then set data.updated to empty string. But moment model doesn't like empty string.

@curbengh
Copy link
Contributor Author

I'm not very keen optional_updated naming, I initially used strict_updated. Any suggestion welcomed.

@curbengh curbengh added this to the Candidate next release milestone Nov 30, 2019
@curbengh curbengh removed this from the 4.2.0 milestone Dec 18, 2019
@SukkaW
Copy link
Member

SukkaW commented Dec 19, 2019

If the Breaking Changes is inevitable, then we might move it to Hexo 5.0.0.

Also an idea is to merge config.use_date_for_updated and config.optional_updated into config.updated_option. We could have options like mtime, date, empty.

@curbengh curbengh added this to the 5.0.0 milestone Dec 19, 2019
@curbengh
Copy link
Contributor Author

curbengh commented Dec 19, 2019

If the Breaking Changes is inevitable, then we might move it to Hexo 5.0.0.

Added to v5 milestone

Also an idea is to merge config.use_date_for_updated and config.optional_updated into config.updated_option. We could have options like mtime, date, empty.

+1, I'll create config.updated_option in a separate PR.

@SukkaW SukkaW mentioned this pull request Jan 8, 2020
@SukkaW SukkaW mentioned this pull request Apr 27, 2020
2 tasks
@SukkaW SukkaW removed this from the 5.0.0 milestone Jun 20, 2020
@curbengh
Copy link
Contributor Author

Superseded by #4278.

@curbengh curbengh closed this Jun 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants