search

qwikifiers / qwik-nx

Nx plugin for Qwik
131 stars 24 forks source link

feat(qwik-nx): deno integration #181

Closed faileon closed 1 year ago

faileon commented 1 year ago

What is it?

Description

Adds integration for deployment to Deno Deploy similar to the qwi-cli deno integration

Use cases and why

Checklist:

nx-cloud[bot] commented 1 year ago

☁️ Nx Cloud Report

Attention: This version of the Nx Cloud GitHub bot will cease to function on July 1st, 2023. An organization admin can update your integration here.

We didn't find any information for the current pull request with the commit a945f693d6c36701b743ce312a46cf63e2fab47a. You might need to set the 'NX_BRANCH' environment variable in your CI pipeline.

Check the Nx Cloud Github Integration documentation for more information.

Sent with 💌 from NxCloud.

dmitry-stepanenko commented 1 year ago

Hey @faileon , thank you so much for the PR, I will review shortly

faileon commented 1 year ago

Thanks for the awesome contribution @faileon! Noticed a couple of things.

Also, it would be preferable to have the new generator covered by e2e tests similarly to https://github.com/qwikifiers/qwik-nx/blob/main/e2e/qwik-nx-e2e/tests/qwik-nx-cloudflare.spec.ts. At the same time I understand it might be a bit nuanced as those would rely on the deno runtime, which should be installed separately. Do you want to try doing that? If not, I can take care of this (at least try to, this use case is specific indeed)

Thanks for the review @dmitry-stepanenko. I already have deno in my dev environment so I will gladly do the e2e tests. I will update the PR soon :)

dmitry-stepanenko commented 1 year ago

Thanks for the review @dmitry-stepanenko. I already have deno in my dev environment so I will gladly do the e2e tests. I will update the PR soon :)

I mean that it will be required to have deno runtime in order to have e2e tests running at all, which can be a bit of a problem. So I'm mostly asking if you think it's ok to install deno programmatically? Because I'm not sure we should..

faileon commented 1 year ago

Thanks for the review @dmitry-stepanenko. I already have deno in my dev environment so I will gladly do the e2e tests. I will update the PR soon :)

I mean that it will be required to have deno runtime in order to have e2e tests running at all, which can be a bit of a problem. So I'm mostly asking if you think it's ok to install deno programmatically? Because I'm not sure we should..

Ah yes, of course...

I am not sure how difficult is ensuring that deno is available on all platforms (local, github actions, ...) for the e2e tests. For example with the GHA we would need to setup another action https://github.com/denoland/setup-deno just for this test.

Personally I wouldn't go this route, at least not now.

dmitry-stepanenko commented 1 year ago

@faileon ok sure, that's what I think either, let's skip it

dmitry-stepanenko commented 1 year ago

Hey @faileon! Would you be able to address comments that I left? If would be really awesome to have this PR merged. If you need any help, please let me know 🙂

faileon commented 1 year ago

Hey @faileon! Would you be able to address comments that I left? If would be really awesome to have this PR merged. If you need any help, please let me know 🙂

Hey, sorry about that, I had the work on local branch only, I pushed the updates to remote just now. 🖖🏿

dmitry-stepanenko commented 1 year ago

Hey @faileon! Would you be able to address comments that I left? If would be really awesome to have this PR merged. If you need any help, please let me know 🙂

Hey, sorry about that, I had the work on local branch only, I pushed the updates to remote just now. 🖖🏿

Ah, I see. Thanks for pushing the fix! Can you also update the lock file? It appears to be broken, CI won't run

faileon commented 1 year ago

Ah, I see. Thanks for pushing the fix! Can you also update the lock file? It appears to be broken, CI won't run

Should be all good now.

faileon commented 1 year ago

I've made some changes locally to this PR that would include the new Deno.server, which should be significantly faster. This did however required updating @builder.io/qwik and @builder.io/qwik-city to 1.2.6.

Also after spending some time with my own repo using Deno, I've tweaked the deploymeny scripts and GHA deploy.yaml template to properly work with SSR (previously it would work with SSG only because it was using a static file server).

I've run the tests pnpm run test qwik-nx successfully:

Test Suites: 23 passed, 23 total Tests: 164 passed, 164 total Snapshots: 152 passed, 152 total Time: 11.719 s, estimated 15 s Ran all test suites.

The e2e tests pnpm test qwik-nx-e2e:e2e still have the same issues.

If the package bump is not a problem I can update this PR. @dmitry-stepanenko

dmitry-stepanenko commented 1 year ago

@faileon sorry wasn't able to get back earlier. Qwik version has to be updated in a several places, I'll bump it to latest and resolve CI failures today/tomorrow

dmitry-stepanenko commented 1 year ago

@all-contributors please add @faileon for code, ideas, tests, docs

allcontributors[bot] commented 1 year ago

@dmitry-stepanenko

I've put up a pull request to add @faileon! :tada: