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

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

Closed
AlynxZhou opened this issue Nov 25, 2019 · 5 comments

Comments

@AlynxZhou
Copy link

AlynxZhou commented Nov 25, 2019

aac8fbb

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
Copy link
Contributor

curbengh commented Nov 25, 2019

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

As part of #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.

hexojs/hexo-theme-unit-test#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
Copy link
Contributor

The EJS snippet as mentioned in hexojs/hexo-theme-unit-test#25, this is how a theme can be made compatible with #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
Copy link
Contributor

Duplicate of #112

@curbengh curbengh marked this as a duplicate of #112 Nov 25, 2019
@AlynxZhou
Copy link
Author

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 👍

@SukkaW
Copy link
Member

SukkaW commented Nov 26, 2019

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants