johannschopplich / nuxt-kql

🫧 Kirby's Query Language API for Nuxt
https://nuxt-kql.byjohann.dev
MIT License
42 stars 2 forks source link

Module improvements #1

Closed pi0 closed 2 years ago

pi0 commented 2 years ago

Hi @johannschopplich! First congratulations on this module. Looks so promising! I've done a quick review and made some options to improve:

johannschopplich commented 2 years ago

Wow, thank you for the code review, @pi0! Foremost, I want to reiterate how impactful I think your work is. You are not only building the base of Nuxt 3, but taking care of the ecosystem. This feels sustainable. The way you comment on PRs etc. is thoughtful and positive. Such a constructive working ethic. Thank you for the care you put in.

I've already implemented most of the suggestions. Thank you!

One particular issue was noted by you swiftly.

I find this little bit complicated. Isn't nitro runtime bundled when we add module path to the transpile? Also why you need to force inline it?

Me too. Took me a while to figure out this workaround. Without this Nitro inlining, I get the following error:

 ERROR  [worker reload] [worker init] Cannot find module '/Users/johann/Git/nuxt-kql-demo/utils.mjs' imported from /Users/johann/Git/nuxt-kql-demo/.nuxt/dev/index.mjs

  at new NodeError (node:internal/errors:372:5)
  at finalizeResolution (node:internal/modules/esm/resolve:437:11)
  at moduleResolve (node:internal/modules/esm/resolve:1009:10)
  at defaultResolve (node:internal/modules/esm/resolve:1218:11)
  at ESMLoader.resolve (node:internal/modules/esm/loader:580:30)
  at ESMLoader.getModuleJob (node:internal/modules/esm/loader:294:18)
  at ModuleWrap.<anonymous> (node:internal/modules/esm/module_job:80:40)
  at link (node:internal/modules/esm/module_job:78:36)

Relative imports to utils doesn't seem to work. The util import in the server API route breaks.

Looks like the line

import { getAuthHeaders } from '../../utils'

… is copied as-is into the final server bundle. The path isn't resolved from within the server route. Do you have another suggestion on how to fix this?

johannschopplich commented 2 years ago

Closing this, as all issues have been resolved, except inlining the module into nitro, which seems to be necessary and has been found in other modules as well, like nuxt-zero-js.