vatesfr / xen-orchestra

The global orchestration solution to manage and backup XCP-ng and XenServer.
https://xen-orchestra.com
Other
741 stars 259 forks source link

fix(web-core): devDependencies → dependencies + peerDependencies #7707

Closed pdonias closed 2 weeks ago

pdonias commented 1 month ago

Description

web-core is published as a library so its dependencies need to be installed when web installs its own dependencies

Checklist

julien-f commented 3 weeks ago

Any idea why the CI is no longer passing?

ByScripts commented 3 weeks ago

Any idea why the CI is no longer passing?

I'm taking a look

ByScripts commented 3 weeks ago

Ok, so here is what I understood after reading about peerDependencies.

So, concerning Web Core:

  1. It is meant to be used in a Vue app and this will want to use Vue, Vue I18n, Vue Router and Pinia from the host. So these should be added as peerDependencies.
  2. It also brings its own required deps (FontAwesome, NoVNC, LoDash etc.). These one should be added in dependencies
  3. At last, we also need to tell which deps are required while developing the lib (i.e., out of the context of its parent). These are @types/*, peer deps (Vue, Pinia etc.) and dev tools. These should be added to devDependencies

Without certainty, maybe the CI error is due to the fact that some required dev deps have been removed (I don't get the error on local dev).

@pdonias Could you try this version:

  "dependencies": {
    "@fontsource/poppins": "^5.0.14",
    "@fortawesome/fontawesome-common-types": "^6.5.1",
    "@fortawesome/free-regular-svg-icons": "^6.5.1",
    "@fortawesome/free-solid-svg-icons": "^6.5.1",
    "@fortawesome/vue-fontawesome": "^3.0.5",
    "@novnc/novnc": "^1.4.0",
    "@vueuse/core": "^10.7.1",
    "@vueuse/shared": "^10.7.1",
    "iterable-backoff": "^0.1.0",
    "lodash-es": "^4.17.21",
    "placement.js": "^1.0.0-beta.5"
  },
  "peerDependencies": {
    "pinia": "^2.1.7",
    "vue": "^3.4.13",
    "vue-i18n": "^9.9.0",
    "vue-router": "^4.2.5"
  },
  "devDependencies": {
    "@types/lodash-es": "^4.17.12",
    "@types/novnc__novnc": "^1.3.5",
    "@vue/tsconfig": "^0.5.1",
    "pinia": "^2.1.7",
    "vue": "^3.4.13",
    "vue-i18n": "^9.9.0",
    "vue-router": "^4.2.5"
  },
pdonias commented 3 weeks ago

I agree about @types/*, etc., I should have put them in devDependencies in the first place. But for peer dependencies, it feels that there should be a better way than duplicating them in devDependencies.

ByScripts commented 3 weeks ago

I accidentally removed "@vueuse/shared": "^10.7.1", in my comment.

I re-added it later, but you were quicker than me ^^

ByScripts commented 3 weeks ago

@pdonias Your last commit removed again Vue, VueRouter, Pinia etc. from devDependencies, so the error came back.

Would it be possible to keep them in devDependencies for now and maybe find a better solution later?

pdonias commented 3 weeks ago

Yes, my last commit was just a test, which is why I switched the PR back to Draft. @julien-f wanted to see which kind of errors were triggered by CI when those packages were not in devDependencies.

But indeed so far, the only way I've seen it work is if the packages are both in peerDependencies and devDependencies. If it's ok with you @ByScripts and @julien-f, I can revert the last commit and we can merge this to at least fix the CI and then we can keep investigating to see if there's a better way to do this?

ByScripts commented 3 weeks ago

Ok for me.