hexojs / hexo-generator-feed

Feed generator for Hexo.
http://hexo.io
MIT License
574 stars 105 forks source link

Writing back `path` to config breaks theme compatibility when path is a string. #115

Closed AlynxZhou closed 4 years ago

AlynxZhou commented 4 years ago

https://github.com/hexojs/hexo-generator-feed/commit/aac8fbb9b0ac82b7334f37b078c7aba01d5c3bb7

This commit rewrite path of this module back into hexo.config.feed, and refer to README.md, hexo.config.feed.path can be (and most time is just) a string rather an array.

I can understand that it is easier to handle via making path an array in code, but for themes assuming it is a string, and telling users to set a string in config, it is confusing that a string in _config.yml becomes an array. Plugins should not edit config, config is written by users, so it should keep the same as _config.yml in most cases.

I spent 2 hours with some errors showing that path.startsWith is not a function from url_for, because I just pass hexo.config.feed.path, which is a string in _config.yml, I only find this after I decide to inject a console.log into url_for.

Please either remove those lines, or at least add some developing API docs in your README.md, to let theme authors know no matter which type you write in _config.yml, hexo.config.feed.path will always be an array.

curbengh commented 4 years ago

it is confusing that a string in _config.yml becomes an array

As part of https://github.com/hexojs/hexo-generator-feed/pull/100, it is now possible for this plugin to output both rss2 and atom.

feed:
  type:
    - atom
    - rss2

Thus, type: can be a string or array.

https://github.com/hexojs/hexo-theme-unit-test/pull/25 includes a code snippet for theme to be compatible with type: in array and string.

at least add some developing API docs in your README.md, to let theme authors know no matter which type you write in _config.yml, hexo.config.feed.path will always be an array.

A theme shouldn't expect a plugin's config to be consistent. If a theme wants to use a plugin's config, it is the theme's responsibility to check for compatibility and add the relevant workaround to the theme, not the other way around.

Besides, this plugin is not expected to be used as some kind of API. A theme using config of this plugin is outside the use case of this plugin. The config of this plugin is never intended to be used by a theme nor other plugins.

I spent 2 hours with some errors showing that path.startsWith is not a function from url_for, because I just pass hexo.config.feed.path

The current readme already shows it is possible for path: to be an array.

Specify ['atom', 'rss2'] to output both types.


This plugin needs to write back to user config to ensure consistency. When it is not consistent, it can break this feature especially this line.

curbengh commented 4 years ago

The EJS snippet as mentioned in https://github.com/hexojs/hexo-theme-unit-test/pull/25, this is how a theme can be made compatible with https://github.com/hexojs/hexo-generator-feed/pull/100,

    <% if (config.feed) { %>
      <% if (config.feed.type.length && config.feed.path.length) { %>
        <% if (typeof config.feed.type === 'string' )) { %>
          <link rel="alternate" type="application/<%- config.feed.type.replace(/2$/, '') %>+xml" title="<%= config.title %>" href="<%- url_for(config.feed.path) %>">
        <% } else { %>
          <% for (const i in config.feed.type) { %>
            <link rel="alternate" type="application/<%- config.feed.type[i].replace(/2$/, '') %>+xml" title="<%= config.title %>" href="<%- url_for(config.feed.path[i]) %>">
          <% } %>
        <% } %>
      <% } %>
    <% } %>
curbengh commented 4 years ago

Duplicate of https://github.com/hexojs/hexo-generator-feed/issues/112

AlynxZhou commented 4 years ago

It's better to check if array or string, but for some template engine it's not so easy to call Array.isArray() like nunjucks. What I am considering is that, if user choose not to set an array in _config.yml, then we'd better not make it an array, so it's easier to refer to their config instead of reading code.

Anyway, thank you for your work :+1:

SukkaW commented 4 years ago

@curbengh

Besides, this plugin is not expected to be used as some kind of API. A theme using config of this plugin is outside the use case of this plugin. The config of this plugin is never intended to be used by a theme nor other plugins.

Since the config format of hexo-generator-feed hasn't been changes for nearly 6 years, writing new format of config back to hexo.config should be considered as a Breaking Change.

In fact, this version of hexo-generator-feed has nearly breaks all the themes that support RSS.