algolia / gatsby-plugin-algolia

A plugin to push to Algolia based on graphQl queries
https://yarn.pm/gatsby-plugin-algolia
Apache License 2.0
177 stars 45 forks source link

fix: Correct `reporter` usage + set correct peerDep, engines #127

Closed LekoArts closed 3 years ago

LekoArts commented 3 years ago

Hello, Gatsby maintainer here 👋

While looking at the plugin I noticed that the peerDependency is set incorrectly. It should be a dependency on gatsby and not gatsby-cli. We're in the process of providing more helpful information on the /plugins page of our website and for that we need plugins to set their peerDependencies correctly/more specific.

I've also updated the minimum Node version as v3 requires at least 12.13.0.

Thanks!

Fixes https://github.com/algolia/gatsby-plugin-algolia/issues/123 Fixes https://github.com/algolia/gatsby-plugin-algolia/issues/126 Fixes https://github.com/algolia/gatsby-plugin-algolia/issues/97

Haroenv commented 3 years ago

Thanks @LekoArts! I noticed in https://github.com/algolia/gatsby-plugin-algolia/issues/123 that the import for the reporter:

https://github.com/algolia/gatsby-plugin-algolia/blob/c909df74c3a41849dd4f0305e52205386c8b089a/gatsby-node.js#L3

possibly might not work with v3. Do you have any ideas on why that could be / the better alternative?

Haroenv commented 3 years ago

Does Gatsby think it's interesting to move this package to the Gatsby monorepo instead of this repo?

LekoArts commented 3 years ago

Hi!

Yeah, the reporter shouldn't be required from gatsby-cli but used like here: https://www.gatsbyjs.com/docs/reference/config-files/node-api-helpers/#reporter

It gets added to the APIs:

exports.onPostBuild = ({ reporter }) => {
  reporter.info(`Your Gatsby site has been built!`)
}

Generally speaking we currently don't want to add new packages to our monorepo, we only maintain critical plugins and let companies maintain their own ones :)


I'll adjust the PR to also change the reporter then :)

LekoArts commented 3 years ago

https://github.com/algolia/gatsby-plugin-algolia/issues/97 proposes the same (correct) thing :)

Haroenv commented 3 years ago

Ah I was confused with it all Lennart, thanks for updating the PR with the fix!

LekoArts commented 3 years ago

This code is untested so far, would appreciate if you can give it a test spin (I don't have any algolia set up) :)

LekoArts commented 3 years ago

Thanks for the quick merge! As I said, please test it before publishing it 👍

Haroenv commented 3 years ago

I had to update the example to v2, but once that was done it all worked, thanks @LekoArts

LekoArts commented 3 years ago

Ideally you should upgrade to v3 😅 But if it works on v2 I'm confident that v3 will also work (but better safe than sorry)

Haroenv commented 3 years ago

yes, I'll do it soon!