search

scripting / rss

JavaScript code to build an RSS feed from a standardized structure.
11 stars 0 forks source link

cloudPing by default calls /pleaseNotify #1

Closed andrewshell closed 2 years ago

andrewshell commented 2 years ago

In the function cloudPing, if urlServer is undefined, it generates its own urlServer from rssCloudDefaults which defines the path as "/pleaseNotify".

This is the only place where rssCloudDefaults is used, and if the function is supposed to ping the rssCloud server, the path should be "/ping".

scripting commented 2 years ago

@andrewshell -- what's the upshot -- do you think this code should change?

andrewshell commented 2 years ago

The upshot is if someone calls that function expecting it to ping the server, it currently won't when urlServer is undefined.

Ideally, the function would just require the urlServer parameter and not accept undefined as a value.

andrewshell commented 2 years ago

Or just remove the function cloudPing entirely so daverss doesn't need to depend on request. Then it's just a library for generating RSS feeds.

scripting commented 2 years ago

What’s the problem? I haven’t touched this code in two years, I’m certainly not going to break things. Are you getting errant calls on your server?

andrewshell commented 2 years ago

That's fine. I was building a project that used this library and I noticed the bug. My guess is nobody uses that function with undefined so it's not a big deal.

scripting commented 2 years ago

I did a search of my own code and find that rss.cloudPing is indeed called with undefined as the first param, in fact it's the normal way to call it because most apps don't have an opinion about which rssCloud server to use, they want the default.

So here's what I'm going to do.

There will be no breakage, because the previous implementation with /pleaseNotify as the path didn't work.

I also did a light code review, and have to say there is other code here that makes even less sense, but it does work when you use it to generate RSS feeds, so I'm going to stick with it. I have bigger fish to fry right now.

Also the reason cloudPing is in this package is the next thing you want to do after publishing a feed is ping the rssCloud server. So this is where it should be. It makes no sense to take it out. I understand you probably don't like the deprecation warning for request, but I actually kind of like it because I think it's totally nuts to even conceive of taking such a core routine out of the NPM feature set. For the first N years this was the only way to do a request, now supposedly it won't work anymore. I'll believe it when I see it. ;-)

scripting commented 2 years ago

Done.

https://github.com/scripting/rss/blob/master/daverss.js#L38

andrewshell commented 2 years ago

That looks good. That should fix it for anybody using the function.