Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add --internal-disable-edge-functions flag to skip Deno setup #6495

Merged
merged 19 commits into from
Apr 8, 2024

Conversation

Skn0tt
Copy link
Member

@Skn0tt Skn0tt commented Apr 5, 2024

@Skn0tt Skn0tt requested a review from seancdavis April 5, 2024 08:49
@Skn0tt Skn0tt self-assigned this Apr 5, 2024
@Skn0tt Skn0tt requested a review from a team as a code owner April 5, 2024 08:49
Copy link

github-actions bot commented Apr 5, 2024

📊 Benchmark results

Comparing with bc24f77

  • Dependency count: 1,312 (no change)
  • Package size: 294 MB ⬇️ 0.00% decrease vs. bc24f77
  • Number of ts-expect-error directives: 1,008 ⬇️ 0.79% decrease vs. bc24f77

@Skn0tt Skn0tt requested a review from a team as a code owner April 5, 2024 09:37
Copy link
Contributor

@seancdavis seancdavis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a few suggestions to change "doesnt" to "doesn't". Didn't review the functionality or the code in depth.

docs/commands/dev.md Outdated Show resolved Hide resolved
docs/commands/serve.md Outdated Show resolved Hide resolved
src/commands/dev/dev.ts Outdated Show resolved Hide resolved
src/commands/serve/index.ts Outdated Show resolved Hide resolved
Skn0tt and others added 4 commits April 8, 2024 11:30
Co-authored-by: Sean C Davis <scdavis41@gmail.com>
Co-authored-by: Sean C Davis <scdavis41@gmail.com>
Co-authored-by: Sean C Davis <scdavis41@gmail.com>
Co-authored-by: Sean C Davis <scdavis41@gmail.com>
@Skn0tt Skn0tt dismissed seancdavis’s stale review April 8, 2024 09:31

changes addressed

@@ -271,6 +272,7 @@ export const createDevCommand = (program: BaseCommand) => {
.option('-d ,--dir <path>', 'dir with static files')
.option('-f ,--functions <folder>', 'specify a functions folder to serve')
.option('-o ,--offline', 'disables any features that require network access')
.option('--disable-edge-functions', "disables edge functions. use this if your environment doesn't support Deno")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should treat this as a customer-facing flag. The only use case we have for it is purely internal, and we haven't fully considered the experience people will get if they disable edge functions. I suggest we use .hideHelp() here (example) so it's removed from the docs, and I would even consider renaming it to something like --internal-disable-edge-functions. That way we're not committing to indefinitely supporting something we're just exploring at this point.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's a very fair point. implemented in 6540b87

@eduardoboucas eduardoboucas changed the title feat: add --disable-edge-functions flag to skip Deno setup feat: add --internal-disable-edge-functions flag to skip Deno setup Apr 8, 2024
@Skn0tt Skn0tt enabled auto-merge (squash) April 8, 2024 10:20
@Skn0tt Skn0tt merged commit a6f0f45 into main Apr 8, 2024
40 checks passed
@Skn0tt Skn0tt deleted the cli-start-without-deno-download branch April 8, 2024 10:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants