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

routeDiscoveryDone Type and Order Posts #15

Merged
merged 22 commits into from
Nov 26, 2020
Merged

routeDiscoveryDone Type and Order Posts #15

merged 22 commits into from
Nov 26, 2020

Conversation

pjlamb12
Copy link
Contributor

feat: convert to routeDiscoveryDone

Convert the plugin to use routeDiscoveryDone instead of render so it runs once instead of once per route
add the option to order by newest first or oldest first

Scully doesn't seem to pass the dates for the route data correctly. The dates are an empty object. I have asked on Gitter/Discord for clarification on this.

Breaking Changes Plugin won't be run as a postRenderer for a route
Closes #13 and #14

Convert the plugin to use routeDiscoveryDone instead of render so it runs once instead of once per route
add the option to order by newest first or oldest first

Closes #13 and #14
@pjlamb12
Copy link
Contributor Author

I'm working on getting the tests fixed on this.

Updated the docs to show how to actually use the plugin currently
changed the check for certain attributes on the route data
only include routes that include /blog or the provided slug from the config
@pjlamb12
Copy link
Contributor Author

I think the last error is unrelated to the changes I've made, but I'm trying to hear back from the Scully Team.

@pjlamb12
Copy link
Contributor Author

I figured out what the last error is. The last error is that when the RSS feed is to be written, the date.toUTCString function fails because the date fields on the HandledRoute are empty objects for some reason. I'm not sure why they're being returned as empty objects, currently.

@marcjulian
Copy link
Member

@pjlamb12 thanks for looking into improving the RSS feed plugin!! I tried it out locally as well and for some reason the publishedAt and updatedAt dates are empty. I thought it might be because scully deps was install as the beta version. After updating it to latest it still fails, updates are added to master.

Here is an example for another routeDiscoveryDone plugin, maybe this helps a bit https://github.com/gammastream/scully-plugins/tree/master/projects/scully-plugin-sitemap

@marcjulian
Copy link
Member

marcjulian commented Nov 23, 2020

Changing the plugin to allDone has the same result as publishedAt and updatedAt are empty objects.

Markdown Frontmatter

---
title: Food
description: Food description
publishedAt: 2020-03-25T10:12:00.000Z
updatedAt: 2020-03-25T10:12:00.000Z
published: true
tags: [lazy-images]
authors:
  - Bruce Lee
---

# Food

HandledRoute in routeDiscoveryDone and allDone

{
  postRenderers: [ 'fouc', 'seoHrefOptimise', 'lazyImages', 'mediumZoom' ],
  route: '/blog/2020-03-21-food',
  type: 'contentFolder',
  templateFile: '/Users/m/Dev/notiz/scully-plugins/scully-plugin-lazy-images/content/blog/2020-03-21-food.md',
  data: {
    name: undefined,
    title: 'Food',
    description: 'Food description',
    publishedAt: {},
    updatedAt: {},
    published: true,
    tags: [ 'lazy-images' ],
    authors: [ 'Bruce Lee' ],
    sourceFile: '2020-03-21-food.md'
  },
  config: {
    type: 'contentFolder',
    slug: { folder: './content/blog' },
    postRenderers: [ 'fouc', 'seoHrefOptimise', 'lazyImages', 'mediumZoom' ]
  }
}

@marcjulian
Copy link
Member

Changing the format of the date in the frontmatter like

---
title: Food
description: Food description
publishedAt: 2020-03-25T
updatedAt: 2020-03-25T
published: true
tags: [lazy-images]
authors:
  - Bruce Lee
---

# Food

It is correctly available in the plugin

{
  postRenderers: [ 'fouc', 'seoHrefOptimise', 'lazyImages', 'mediumZoom' ],
  route: '/blog/2020-03-21-food',
  type: 'contentFolder',
  templateFile: '/Users/m/Dev/notiz/scully-plugins/scully-plugin-lazy-images/content/blog/2020-03-21-food.md',
  data: {
    name: undefined,
    title: 'Food',
    description: 'Food description',
    publishedAt: '2020-03-25T',
    updatedAt: '2020-03-25T',
    published: true,
    tags: [ 'lazy-images' ],
    authors: [ 'Bruce Lee' ],
    sourceFile: '2020-03-21-food.md'
  },
  config: {
    type: 'contentFolder',
    slug: { folder: './content/blog' },
    postRenderers: [ 'fouc', 'seoHrefOptimise', 'lazyImages', 'mediumZoom' ]
  }
}

However, now it is not an valid date string.

@pjlamb12
Copy link
Contributor Author

@marcjulian yeah, but if you change the dates to strings, just by wrapping them in quotes, then the string value comes through to the plugin. But the RSS Feed npm package then fails because it can't call a function on the string.

@marcjulian
Copy link
Member

Okay as string it is correctly parsed to the plugin. That leaves us to the question why is the date not correctly available during the plugin process if it is not a string. I saw your discussion in Scully Gitter.

What if the date is a string in the front matter and the plugin transforms that string into a date for the RSS Feed package?

@pjlamb12
Copy link
Contributor Author

I thought about that, but it felt like a bad workaround. The dates shouldn't be empty objects and finding the solution there felt better. But I'm open to whatever. If we can't figure out what's going on in scully, I guess we'd have to do that.

@marcjulian
Copy link
Member

Yes building a workaround doesn't feel right, would be better if scully doesn't parse the dates as empty objects. Let's see if we can find something out with scully. If that doesn't work out we could start over with the workaround. What do you think would be the best approach?

@pjlamb12
Copy link
Contributor Author

I think the best workaround approach would be having those dates be strings, like:

publishedAt: '2020-01-01'

And then in the createFeedItemFromRoute function in the rss.js file, we could convert the date from a string to a date:

date: route.data.updatedAt ? new Date(route.data.updatedAt) : new Date(route.data.publishedAt)

What do you think?

@marcjulian
Copy link
Member

Yes sounds good. What if we want to keep the time as well?

publishedAt: '2020-03-25T10:12:00.000Z'

And then we use for example dayjs to convert it to a date

date: route.data.updatedAt ? dayjs(route.data.updatedAt) : dayjs(route.data.publishedAt)

@pjlamb12
Copy link
Contributor Author

That would be great too. Works for me.

@marcjulian
Copy link
Member

I just tried it out and it would work with dayjs like so

date: route.data.updatedAt ? dayjs(route.data.updatedAt).toDate() : dayjs(route.data.publishedAt).toDate()

@pjlamb12
Copy link
Contributor Author

what happens if route.data.updatedAt or route.data.publishedAt is already a date object? Because if it works, then we could just go this route and if/when Scully passes the date as an actual date object instead of an empty object, then the plugin would still work.

@marcjulian
Copy link
Member

I think dayjs would be fine either way with the string or the actual date object.

@pjlamb12
Copy link
Contributor Author

Maybe we should just go ahead and add dayjs and the plugin would work either way and the release could be done.

@marcjulian
Copy link
Member

marcjulian commented Nov 23, 2020

I tried it out on the master branch. The routes value is as following

{
  postRenderers: [ 'fouc', 'seoHrefOptimise', 'lazyImages', 'mediumZoom', 'rss' ],
  route: '/blog/2020-03-21-food',
  type: 'contentFolder',
  templateFile: '/Users/m/Dev/notiz/scully-plugins/scully-plugin-lazy-images/content/blog/2020-03-21-food.md',
  data: {
    name: undefined,
    title: 'Food',
    description: 'Food description',
    publishedAt: 2020-03-25T10:12:00.000Z,
    updatedAt: 2020-03-25T10:12:00.000Z,
    published: true,
    tags: [ 'lazy-images' ],
    authors: [ 'Bruce Lee' ],
    sourceFile: '2020-03-21-food.md'
  },
  config: {
    type: 'contentFolder',
    slug: { folder: './content/blog' },
    postRenderers: [ 'fouc', 'seoHrefOptimise', 'lazyImages', 'mediumZoom', 'rss' ]
  }
}

I installed dayjs and wrapped the date as described above and it works fine.

@marcjulian
Copy link
Member

Do you want to add dayjs to the plugin and push again?

@pjlamb12
Copy link
Contributor Author

Yes I will do that.

@marcjulian
Copy link
Member

Thanks for looking into this 💯 much appreciated!

@pjlamb12
Copy link
Contributor Author

How do I import dayjs into the plugin's rss.js file? I npm installed it, and required dayjs at the top but that isn't working.

@marcjulian
Copy link
Member

marcjulian commented Nov 23, 2020

const dayjs = require("dayjs");

use dayjs to convert strings to dates, or date objects to dates, for the rss plugin
added dayjs as a dependency for the plugin
added myself as a contributor to the plugin
@marcjulian
Copy link
Member

Do you mind adding dayjs to the rss plugin and not in the root package.json?

@marcjulian
Copy link
Member

Yes might be because of tailwind which is added to the root dependencies. Let me have a look, sorry for the inconvenience

@marcjulian
Copy link
Member

Works now. What about adding some logs for the user to the plugin? How about

Started @notiz/scully-plugin-rss
Generating RSS Feed for 2 blog posts
✅ Created ./dist/static/feed.xml
✅ Created ./dist/static/feed.atom
✅ Created ./dist/static/feed.atom
Finished @notiz/scully-plugin-rss

Screenshot 2020-11-23 at 19 23 47

@pjlamb12
Copy link
Contributor Author

The log output was just pushed.

@marcjulian
Copy link
Member

@pjlamb12 thanks for this PR. I will merge it tomorrow and update the readme about the breaking changes and the new config option.

@pjlamb12
Copy link
Contributor Author

Sweet! I was just thinking, I wonder if we should add a line in the README about putting the dates in strings for now.

@pjlamb12
Copy link
Contributor Author

Just a heads up, here's a fix in Scully for this issue: scullyio/scully#1140

@marcjulian
Copy link
Member

@pjlamb12 awesome 💯 ! We can wait for it to be release and than we can remove dayjs and not introduce a breaking change.

@pjlamb12
Copy link
Contributor Author

pjlamb12 commented Nov 24, 2020

Although it will still be a breaking change since it's not a render plugin anymore, right?

@marcjulian
Copy link
Member

Yes true. I will make that clear anyways and it will require at least @scullyio/[email protected]

@marcjulian marcjulian merged commit fbe9141 into notiz-dev:master Nov 26, 2020
@marcjulian
Copy link
Member

@pjlamb12 thanks for your help!! 👍 new PRs are always welcome.

@marcjulian marcjulian linked an issue Nov 26, 2020 that may be closed by this pull request
@pjlamb12
Copy link
Contributor Author

No problem, glad to help, and I'm excited to implement this in my own site! Thank you for creating the plugin and finishing up the pr!

@marcjulian
Copy link
Member

Your welcome. Release is published! I bumped it to 1.0.0 as it is a new plugin type. If you find anything missing in the readme or anywhere let me know!

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.

RSS: Refactor plugin type to routeDiscoveryDone RSS: Add Option to Reverse Items in the Feed
2 participants