nuxt-modules / algolia

🔎 Algolia module for Nuxt
https://algolia.nuxtjs.org/
MIT License
185 stars 32 forks source link

Monorepo failures due to thrown errors #153

Closed genu closed 9 months ago

genu commented 1 year ago

https://github.com/nuxt-modules/algolia/blob/d0af3d9b179d4c3a94d6b5133b8fefab638a5fe6/src/module.ts#L71-L86

I don't think Errors should be thrown here because it will always fail sibling projects. This happens because in a monorepo the nuxt postinstall script will always run, which will throw these errors for every project except the nuxt project due to the missing env vars.

Instead of throwing here, warning should be emitted.

Baroshem commented 1 year ago

Hey,

Thanks for reporting this.

I intentionally decided to throw errors here because if you run the module without required options (such as apiKey and applicationId) the whole module will not work. Because of that, there is no reason for showing warnings in the console as user may easily miss it and start wondering why the app does not work correctly.

I wouldn't worry about the post-install too much :)

genu commented 1 year ago

I wouldn't worry about the post-install too much :)

postinstall is the default behavior for nuxt to generate its types inside of .nuxt, and it will automatically execute the module.ts in this module.

The errors are thrown regardless of whether we are using the module or not.

Its the sibling projects that break, not that project using Algolia.

Your reasoning is correct, but the way nuxt works is causing those issues.

At the very least, the logger package should be use from @nuxt/kit for outputting.

Have a look at the tailwind package for reference: https://github.com/nuxt-modules/tailwindcss/blob/main/src/module.ts#L145-L150

  try {
       _tailwindConfig = requireModule(configPath, { clearCache: true })
   } catch (e) {
        logger.warn(`Failed to load Tailwind config at: \`./${relative(nuxt.options.rootDir, configPath)}\``, e)
   }
Baroshem commented 1 year ago

I see, that makes sense.

Would you mind if I convert this task into a feature request to replace the current error throwing functionality into native nuxt logger with warns?