quasarframework / quasar-testing

Testing Harness App Extensions for the Quasar Framework 2.0+
https://testing.quasar.dev
MIT License
180 stars 65 forks source link

Improve error message on not found of cypress quasar dev server #383

Closed hinogi closed 4 days ago

hinogi commented 1 week ago

It would be helpful if any of those checks would throw an error that would indicate either what package json was searched and where, where it was searched dependencies/devDependencies, and the name of the package.

https://github.com/quasarframework/quasar-testing/blob/cd092e0380564c3f0fec94650acac8505068cd8a/packages/e2e-cypress/src/helpers/cct-dev-server/index.ts#L119

alternatively it would be great if there would be an argument where you can force webpack or vite if you have a monorepo or other kind of setup that is not expected.

IlCallo commented 1 week ago

I'm sorry, but this request doesn't really seem to make sense

If you're installing an AE, you MUST have a @quasar/app-vite or @quasar/app-webpack in your package.json, and it should always be in devDependencies

There aren't other possibilities for it to work properly and it's meant to fail-fast if you play around and change the expected structure The check and error message should already be thrown when you run quasar ext add @quasar/testing-xxx to install the AE, and the expected structure isn't found

Even in monorepos, each Quasar app should have their own @quasar/app-vite/@quasar/app-webpack, and possibly their quasar and vue related dependencies

Bottom line: moving all dependencies to a root package.json in a monorepo is often an anti-pattern and we won't support it for Quasar app packages

Mind to expand on which is your use case, why you ended up there and which principles you followed while getting there?

hinogi commented 1 week ago

I guess these reflect the dependency approach https://nx.dev/concepts/decisions/dependency-management

Maybe it would also be enough to mention how to handle quasar in monorepos like a caveats section or something.

hinogi commented 1 week ago

And sorry if it doesn't make sense to let the user know where a possible problem might originate from. Sure, ever dev can dig into the source files and find out for themselves just like I did. The intention was to make a generic thrown error into a meaningful error that tells the user about possible ways to fix the error. Since there is no documentation on what is necessary to be able to run the injected dev server on a lower level. And since only the quasar cli might take care of providing the actual things necessary which might get under the bus when someone removes unused package.json files from a monorep for example. Anyway, if you don't feel like doing it, okay for me, I know the workaround and that is enough for me, my intention was to let other benefit of it. If that is against the goal of the package so be it, no harm done.

IlCallo commented 4 days ago

I guess these reflect the dependency approach

Correct, we manage dependencies like explained in this section: https://nx.dev/concepts/decisions/dependency-management#independently-maintained-dependencies

Maybe it would also be enough to mention how to handle quasar in monorepos like a caveats section or something.

Quasar main repo itself is a monorepo and we ensured that everything works fine when a Quasar project is added into a monorepo In my company we always use Quasar into monorepos and every time we find any problem we report it and submit a PR with the fix

From what you wrote early on, it seems like you're using NX, of which we (the whole Quasar team) don't have experience and which isn't officially supported From your initial post, it wasn't even clear you were using it

If you wanna list here the problems you faced and how you got around it, we can add a section about NX compatibility caveats on Quasar docs

And sorry if it doesn't make sense to let the user know where a possible problem might originate from [...] Since there is no documentation on what is necessary to be able to run the injected dev server on a lower level

While I agree with you that having nice error messages for every case results in a nicer DX, only public APIs should be documented, and this one isn't Doing so would lead to more people relying on them and that would create a problem in case we change how the system works (e.g. if we decide to move all app extensions to "dependencies" instead of "devDependencies")

To be clear: adding a single error message isn't not a problem, but it's not sustainable to take into account every possible user tampering with Quasar assumptions, which are the de-facto standards in the ecosystem, and write error messages for everything. We rely on Node lower level error messages for those cases.


I hope I clarified that it's not a matter of feeling like doing it or not: it took me 4x time to explain it to you why we won't do that rather than just do it I may have answered harshly in my first comment, but your use case wasn't clear and you provided little to no context about your setup, while asking for this change

That said, thank you for reporting, writing this issue could help other people using NX (if you're using that) to find a solution If enough people ask for it, we may find a way to properly support NX in the first place Up until then, please keep reporting here the quirks you find with NX and how you solved them