Improvement suggestions

drunomics / nuxtjs-drupal-ce

A Nuxt module to easily connect Drupal via custom elements.

https://lupus-decoupled.org/

MIT License

22 stars 4 forks source link

Improvement suggestions #53

Closed pi0 closed 2 years ago

pi0 commented 2 years ago

Hi and thanks for the nice module. Adding some improvement suggestions:

Indirectly depending on axios module is not a good idea as options like baseUrl can easily be conflicting with user config. I would recommend migrating and directly use ohmyfetch to be consistent with Nuxt3
Please consider using explicit .mjs when using import syntax (also for addTemplate with plugin.mjs) and avoid mixing CJS (require) with ESM. More info: https://v3.nuxtjs.org/concepts/esm
It seems you are forcely changing vue alias even if already set (https://github.com/drunomics/nuxtjs-drupal-ce/blob/9ca4a70ae82881900382a5efd22f4ffe153c5ca9/lib/module.js#L33). It should be something like config.resolve.alias.vue = config.resolve.alias.vue|| 'vue/dist/vue.common'
Would be also nice if consider trying to support Nuxt 2 Bridge. More info: https://v3.nuxtjs.org/getting-started/migration#module-migration

fago commented 2 years ago

@pi0 - thank you for the review & suggestions!

Indirectly depending on axios module is not a good idea as options like baseUrl can easily be conflicting with user config. I would recommend migrating and directly use ohmyfetch to be consistent with Nuxt3

I'm not sure I can follow the problem with user config, could you elaborate on that? We are creating another axios instance below our $drupal helper object, does that solve it? see https://github.com/drunomics/nuxtjs-drupal-ce/blob/1.x/lib/plugin.js#L216

yeah, ohmyfetch and nuxt-brige will be good improvements, but break BC for folks already using the module. I think we should do above fixes, roll a quick stable 1.x and get started with a 2.x version using nuxt bridge and ohmyfetch. I opened #54 for that.

pi0 commented 2 years ago

I'm not sure I can follow the problem with user config, could you elaborate on that? We are creating another axios instance below our $drupal helper object, does that solve it? see https://github.com/drunomics/nuxtjs-drupal-ce/blob/1.x/lib/plugin.js#L216

Interceptors and defaults get inherited. this is a common issue with also have with auth-module and plan to remove this.

yeah, ohmyfetch and nuxt-brige will be good improvements, but break BC for folks already using the module. I think we should do above fixes, roll a quick stable 1.x and get started with a 2.x version using nuxt bridge and ohmyfetch. I opened #54 for that.

A mjavor 2.x version seems best plan 💯

fago commented 2 years ago

Interceptors and defaults get inherited. this is a common issue with also have with auth-module and plan to remove this.

yep, I'm not even sure that's bad in our case. Anyway, as discussed changing that is a 2.x topic!