qwikifiers / qwik-nx

Nx plugin for Qwik
130 stars 23 forks source link

fix(qwik-nx): add missing @nrwl/* packages to qwik-nx to ensure they resolve correctly #114

Closed jaysoo closed 1 year ago

jaysoo commented 1 year ago

What is it?

Description

When generating apps with qwik-nx:app there is an error due to missing @nrwl/linter dependency. It was pulled in previously because of this dependency chain: qwik-nx -> @nrwl/vite -> @nrwl/js -> @nrwl/linter. In Nx 15.9.0, we removed the @nrwl/js -> @nrwl/devkit dependency so the latter package is no longer installed.

Adding these missing packages removes the reliance on them being hoisted by a package manager.

Use cases and why

Screenshots/Demo

Screenshot 2023-03-31 at 8 42 37 AM

Checklist:

dmitry-stepanenko commented 1 year ago

Thank you so much for catching this early @jaysoo !

jaysoo commented 1 year ago

I'm not sure why the e2e tests are failing -- they pass locally. Happy to fix it, or if you want to open another PR with the same change that works as well. :)

dmitry-stepanenko commented 1 year ago

I'm not sure why the e2e tests are failing -- they pass locally. Happy to fix it, or if you want to open another PR with the same change that works as well. :)

this is a bit weird tbh 🤔 let me debug it and I'll let you know

dmitry-stepanenko commented 1 year ago

@jaysoo so the problem is that in local e2e tests dependencies can be resolved from both installed packages under /tmp folder or from the main node_modules. I'm not sure why this is not the same on CI, but in this case I'm happy it is not :)

So the reason it is failing is because create-nx-workspace will install packages with --legacy-peer-deps flag, meaning no peer deps are installed. Thus it is required to ensure each of peer dependencies is installed manually, right now only checking for @nrwl/vite. I've added logic to handle this in another PR https://github.com/qwikifiers/qwik-nx/pull/115

Do you think Nx should install peer dependencies for the preset package?

jaysoo commented 1 year ago

Oh I see. Should we move them into dependencies rather than peerDependencies? We can also remove them from package.json completely, and just rely on ensurePackage in the generator that uses them.

One of those options is probably better than having to install peer deps.

dmitry-stepanenko commented 1 year ago

Removing them from peer dependencies is not an option, because it is expected that they're present, only preset generator runs ensurePackage against peer dependencies.

Your point about moving them into dependencies is valid, but there's a problem: when package is built with @nrwl/js:tsc, dependencies are being resolved to a specific version. So that this

"dependencies": {
    "@nrwl/devkit": "^15.8.0",
    "@nrwl/js": "^15.8.0",
    "@nrwl/linter": "^15.8.0",
    "@nrwl/vite": "^15.8.0"
  },

becomes this in the dist folder

"dependencies": {
    "@nrwl/devkit": "15.9.2",
    "@nrwl/js": "15.9.2",
    "@nrwl/linter": "15.9.2",
    "@nrwl/vite": "15.9.2"
  },

Obviously we don't want to have a specific version over there. The e2e check that you saw failed is aimed to catch any issues with this.

Probably will raise a ticket in nx repo about this behavior, not sure if it is valid. Or maybe you know how it can be overcome?

jaysoo commented 1 year ago

dependencies are being resolved to a specific version

Ah yes, I think because the version is matched to what is inside the lockfile. This behavior makes sense for apps since you want exactly the version that matches the workspace, but for libs you'd want to keep the range. I'll make a task to track this.

There is the option of setting --generatePackageJson=false when building for now, and the versions set in the project'spackage.json will be maintained.

dmitry-stepanenko commented 1 year ago

@jaysoo unfortunately generatePackageJson is only applicable for webpack and esbuild executors from what I see in the nx source code. I've tried it now and it doesn't have any effect. There're a few similar issues opened in your repo but I don't see a working solution there :(

dmitry-stepanenko commented 1 year ago

@jaysoo I've moved nrwl packages to dependencies as you suggested https://github.com/qwikifiers/qwik-nx/pull/128, found out there's a updateBuildableProjectDepsInPackageJson flag that prevents pinning of the versions of package's dependencies. Thanks for your help!