Skip to content

Conversation

@maciejpedzich
Copy link

No description provided.

@maciejpedzich maciejpedzich mentioned this pull request Apr 1, 2022
@pi0 pi0 changed the title Merge version 3.0.0 fork into main repository feat!: rewrite with nuxt kit and nuxt 3 support Apr 1, 2022
@pi0
Copy link
Member

pi0 commented Apr 1, 2022

Thanks for the pull request @maciejpedzich. I've made an overall review this is in a nice direction.

  • It is fine to generally have customizations in repo setup. But ideally, it should be as close as possible with module starter to keep it's consistency with the rest of (new) nuxt-community modules and I would like to hear from @manniL preferences as he is the current maintainer.
  • Since we are rewriting the module with Nuxt 3 in mind, ideally should also follow its design goals. Mainly, integration with nitro with an API route to allow runtime generation of sitemap vs on build generation. We can do it later but have it in mind. In the meantime, Avoid async resolve function in nuxt.config and depend on nuxt hooks for extension. (added in review comments)

@TheAlexLichter TheAlexLichter self-requested a review April 6, 2022 14:33
@TheAlexLichter
Copy link
Member

Thanks for the PR! I'll have a look in the next days ☺️

@TheAlexLichter TheAlexLichter self-assigned this Apr 6, 2022
@maciejpedzich
Copy link
Author

Hi @pi0, thanks for the review! Sorry for taking so long, luckily I've found time to properly get down to adjusting the code accordingly with the changes you've requested. I'll let you when I'm done correcting everything.

Copy link
Member

@TheAlexLichter TheAlexLichter left a comment

Choose a reason for hiding this comment

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

Left a few comments! Good job so far ☺️


- name: Install dependencies
if: steps.cache.outputs.cache-hit != 'true'
run: yarn
Copy link
Member

Choose a reason for hiding this comment

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

Is there a specific reason for using npm here? ☺️

run: yarn lint
run: npm run lint

- name: Test
Copy link
Member

Choose a reason for hiding this comment

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

It'd be great to release a tested module (even if not fully tested).


- Nuxt 3 ready
- Support for RSS 2.0, Atom, and JSON formats
- API similar to <a href="https://github.com/nuxt-community/feed-module"><code>@nuxt-communit/feed-module</code></a>, making migration over to <code>@nuxtjs/feed</code> painless
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- API similar to <a href="https://github.com/nuxt-community/feed-module"><code>@nuxt-communit/feed-module</code></a>, making migration over to <code>@nuxtjs/feed</code> painless
- API similar to <a href="https://github.com/nuxt-community/feed-module"><code>@nuxt-community/feed-module</code></a>, making migration over to <code>@nuxtjs/feed</code> painless

<code-block label="NPM" active>

```bash
npm install --save @nuxtjs/feed
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
npm install --save @nuxtjs/feed
npm install @nuxtjs/feed

```ts
import axios from 'axios';

async create(feed) {
Copy link
Member

Choose a reason for hiding this comment

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

I'd suggest printing the feed object here:

feed: {
  async create() { /** the rest **/ }
}

import axios from 'axios';

async create(feed) {
const { data } = await axios.get<{ articles: Article[] }>(
Copy link
Member

Choose a reason for hiding this comment

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

Could we use ohmyfetch or (node) fetch for the example?


## Dynamic module config - factory function

There are cases, where the _static array_ config could turn out to be more WET and less readable/flexible. For instance, what if you needed to generate feeds based on multiple different categories, which you may also want to fetch from an external API?
Copy link
Member

Choose a reason for hiding this comment

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

'WET' might not be clear to everyone. Maybe use something like "repeatable"?

) {
if (typeof options === 'function') {
return options();
} else if (Array.isArray(options)) {
Copy link
Member

Choose a reason for hiding this comment

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

No else needed. Same below.

name: '@nuxtjs/feed',
configKey: 'feed'
},
setup(moduleOptions, nuxt) {
Copy link
Member

Choose a reason for hiding this comment

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

Now that SSG landed, the feed module could provide static feed generation directly too 😋

@maciejpedzich
Copy link
Author

maciejpedzich commented May 18, 2022

@manniL, @pi0 - I've decided to retire my custom Nuxt 3 compatible RSS feed module, and I'm afraid I won't be able to actively support the feed module as a community maintainer because I simply don't have the bandwidth to do it anymore. Feel free to reuse the code as a foundation for the official one.

@TheAlexLichter
Copy link
Member

@manniL, @pi0 - I've decided to retire my custom Nuxt 3 compatible RSS feed module, and I'm afraid I won't be able to actively support the feed module as a community maintainer because I simply don't have the bandwidth to do it anymore. Feel free to reuse the code as a foundation for the official one.

@maciejpedzich Thanks a lot for being that open and for the PR! I will try to move the module to Nuxt3 based on your code in near future (and give appropriate credits) 👍

@entity
Copy link

entity commented May 22, 2022

@manniL, @pi0 - I've decided to retire my custom Nuxt 3 compatible RSS feed module, and I'm afraid I won't be able to actively support the feed module as a community maintainer because I simply don't have the bandwidth to do it anymore. Feel free to reuse the code as a foundation for the official one.

@maciejpedzich Thanks a lot for being that open and for the PR! I will try to move the module to Nuxt3 based on your code in near future (and give appropriate credits) 👍

Hey folks, is there any working sitemap generator these days that supports Nuxt 3 and Nuxt Content V2?

@xlanex6
Copy link

xlanex6 commented May 30, 2022

Hi @manniL

Is there a roadmap, todo list to finish the job initiate by @maciejpedzich ?
I will need RSS feed on my Nuxt 3 project so... I'm ready to spend time and energy to finish what have to be done.

@TheAlexLichter
Copy link
Member

Feel free to check #106. I haven't had the time to continue from here, so whoever wants to take a stab is good to go ☺️

@nozomuikuta nozomuikuta mentioned this pull request Jul 21, 2022
28 tasks
@nozomuikuta nozomuikuta mentioned this pull request Dec 14, 2022
21 tasks
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

Successfully merging this pull request may close these issues.

6 participants