search

docpad / docpad-plugin-paged

Adds support to DocPad for rendering a document into multiple pages.
Other
28 stars 11 forks source link

getPageUrl() name collision with docpad-plugin-services #10

Closed justinbelcher closed 9 years ago

justinbelcher commented 11 years ago

getNextPage() and getPrevPage() were not returning correct urls even though document.page data looked okay, and after debugging I saw that https://github.com/docpad/docpad-plugin-paged/blob/master/src/paged.plugin.coffee#L79 was actually calling the getPageUrl() defined in the services plugin: https://github.com/docpad/docpad-plugin-services/blob/master/src/services.plugin.coffee#L16

I'm able to work around this temporarily by just disabling the services plugin, but you may want to address that collision in case folks need to use both plugins.

kunjee17 commented 11 years ago

I am facing the same issue after introducing service plugin. getPageURL stops working. I think I should disable service plugin and check it out.

kunjee17 commented 11 years ago

Not only that. It is having collision with other pluggins too. Like I tried tagging and first thing it broke is my pagging.

misterdai commented 10 years ago

I'm finding this very problematic as well. Again with the paged and services plugins. One time I ran it and the paged plugin must have loaded after the services plugin, because it started working. But then it went back to use the services version of getPageURL.

I'd suggest providing an alias for getPageURL (e.g. getPagedURL) to alleviate the problem for users of both plugins and prevent breaking it for anyone else. Then, in the main docpad project and supported plugins, think about a way to namespace plugins to try and prevent future conflicts (probably have to wait for a new major release).

misterdai commented 10 years ago

Just noticed, the readme states that the method is actually called "getPagedUrl" but in the code itself and in the code example within the readme, it is called getPageUrl. Which is it actually meant to be? Obviously getPagedUrl would be best and solve the short term conflict with the services plugin (but not the long term possible plugin conflict issue).

StormPooper commented 9 years ago

I'd rather make a breaking change then leave the plugins incompatible, so I think we'll have to go ahead and rename them. I'll look into declaring an alias if it hasn't already been defined to maintain some form of backwards compatibility though.

StormPooper commented 9 years ago

Fixed, will be included in the next release of the plugin. I've made it define getPageUrl if it's not already defined and it warns the user that it's deprecated. If it's already defined, it'll warn the user of this. I think this is the best we can do without detecting specific plugins, which is a rabbit hole I don't want to head down.

StormPooper commented 9 years ago

Fixed in v2.4.0.