skymethod / denoflare

Develop, test, and deploy Cloudflare Workers with Deno.
https://denoflare.dev
MIT License
692 stars 33 forks source link

Allow --no-check option for bundle #20

Closed davesnx closed 2 years ago

davesnx commented 2 years ago

Hi @johnspurlock-skymethod

Awesome project, I enjoy a lot.

Would be possible to configure the params like no-check in https://github.com/skymethod/denoflare/blob/54e1afabea7579fddb1070b5fcbe0ef20894c981/cli/deno_bundle.ts ?

johnspurlock-skymethod commented 2 years ago

Sure thing - do you mean in the exported denoBundle function? Or all the way up to denoflare serve?

serve is usually run for local development, so the thinking was that type checking is most useful at this time - typechecking was also the default from when we were able to use Deno.emit (RIP), which also returned TS errors for display.

If you mean only in the denoBundle function, which other deno bundle options are you wanting to pass in? (besides --no-check)

davesnx commented 2 years ago

I'm more particularly interested in deno bundle with --no-check. It's my first real project with deno and I'm unsure about the number of options that might be needed. I would assume that all options available would make sense, but that's on you to judge.

My use-case/issue is not related to denoflare not having the possibility to add flags, but having that would have made the setup of my project possible. I have 2 deno applications in the same project and the cache for dependencies isn't the smartest while there's some clashing with dependencies from fresh and my other project.

Thanks for the fast response

davesnx commented 2 years ago

Right, the problem is not having 2 deno apps, the problem is with one of the dependencies: https://github.com/octokit/octokit.js/issues/2075

Since I can't bundle it with --no-check I can't deploy either run the worker.

johnspurlock-skymethod commented 2 years ago

So you're getting errors on denoflare push then when you try to deploy your worker to Cloudflare?

That also bundles under the hood before deploying the output as the single script to Cloudflare.

There are actually currently two backends for this bundling process in Denoflare, the default is to call deno bundle behind the scenes (as you're finding out), but you can try passing --bundle module to either push or serve to switch to the newer and more experimental deno_emit userland module backend.

This userland module was provided by the Deno team when they removed Deno.emit from Deno, which Denoflare had been using to bundle. Unfortunately it's not nearly as stable and does not support all imported dependencies.

But... it does not (and cannot) typecheck - so may be just what you need, while you're waiting for the ability to turn off typechecking in Denoflare itself.

davesnx commented 2 years ago

So you're getting errors on denoflare push then when you try to deploy your worker to Cloudflare? Correct, both push and serve.

Tried to use --bundle module to force the deno_emit, but couldn't

error: Uncaught (in promise) Error: Bad name value: module
    if (i < 0) throw new Error(`Bad name value: ${str}`);
                     ^
    at parseNameValue (https://raw.githubusercontent.com/skymethod/denoflare/v0.5.3/cli/cli_command.ts:370:22)
johnspurlock-skymethod commented 2 years ago

Sorry, should have read my own docs

This advanced option should be passed as --bundle backend=module

e.g.

$ denoflare serve /path/to/worker.ts --bundle backend=module

I noticed when trying it again locally that this backend currently requires an absolute file path to the worker (if passing in a worker path instead of a config script name). I'll make a note to improve this - the default deno bundle backend does not have this restriction.

davesnx commented 2 years ago

I coudn't make it work with --bundle backend=module.

I managed to fork and try it out myself (that's one of the great things of deno lol) and managed to add noCheck in deno_bundle: https://github.com/skymethod/denoflare/compare/master...davesnx:denoflare:master

johnspurlock-skymethod commented 2 years ago

Yea that will set the --no-check flag, which is the Deno nuclear option. I'll make a Denoflare change that supports all the checking modes, and provide those as options in serve and push

https://deno.com/blog/v1.22#default-type-checking-behavior-has-been-updated

Right now, deno bundle will only check types for "local" code, not code you import from a remote url. Surprised that didn't work for you since it sounded like it was a 3p dependency that was having typing issues

johnspurlock-skymethod commented 2 years ago

Right, the problem is not having 2 deno apps, the problem is with one of the dependencies: octokit/octokit.js#2075

Since I can't bundle it with --no-check I can't deploy either run the worker.

Looking into this, and I'm able to use octokit just fine in Denoflare, (and latest Deno 1.23.4)

No issues in bundling for serve, nor any type issues testing with calling deno bundle on the source outside of Denoflare

import { Octokit } from "https://cdn.skypack.dev/octokit?dts";
// type-checks
import { Octokit } from "https://esm.sh/octokit@2.0.3";
// type-checks
// but has some dubious runtime code that references the `Deno.` global, which is not available on Cloudflare, and thus not available to worker scripts inside Denoflare.

Do you have a snippet of code you could share that's failing for you?

johnspurlock-skymethod commented 2 years ago

Just pushed commit 5a9b42225d0e8b11f047c9d87c8c1c89bec99348 with the ability to set the type-checking level in the denoBundle helper, as well as in serve and push. Can you reinstall denoflare to this commit hash and try it out?

Since the denoBundle function is meant to be a direct helper for deno bundle (and useful outside of Denoflare), any check, noCheck options are passed directly info the process call.

However, since the current state of Deno is kind of confusing at the moment with both --no-check and --check options that conflict with each other, and very few valid option values (see https://github.com/denoland/deno/blob/main/cli/args/flags.rs), I instead expose the three current type-checking levels as a single optional --bundle advanced option, called check, with all, local, and none as allowed values.

The default is unchanged, serve and push type-checks (local only, no remote deps).

However, you can dial up the type-checking up using --bundle check=all, which will type-check both local and remote, or dial it down using --bundle check=none.

e.g. to disable type-checking

$ denoflare serve my-script --bundle check=none

and to enable type-checking both local and remote deps

$ denoflare serve my-script --bundle check=all
davesnx commented 2 years ago

Do you have a snippet of code you could share that's failing for you? I managed to get rid of Octokit all together, fork it and made direct requests to the methods I care.

Problem went away in my side, but I agree check, no-check is confusing for deno.

Thanks for adding the flag bundle check non, I believe is still useful 👍🏼

johnspurlock-skymethod commented 2 years ago

Absolutely, thanks for bringing it up

johnspurlock-skymethod commented 2 years ago

This change is included in release v0.5.4