Skip to content
This repository has been archived by the owner on Aug 3, 2023. It is now read-only.

Add the ability to preview without opening the browser #816

Merged
merged 8 commits into from
Nov 5, 2019

Conversation

EverlastingBugstopper
Copy link
Contributor

@EverlastingBugstopper EverlastingBugstopper commented Oct 28, 2019

Fixes #256

This PR adds wrangler preview --headless which does not open the browser for preview requests. The tests for wrangler preview now use this flag.

@@ -34,6 +34,7 @@ pub fn preview(
body: Option<String>,
livereload: bool,
verbose: bool,
browser: bool,
Copy link
Contributor

Choose a reason for hiding this comment

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

the flag is "terminal" but this attribute says browser... might be more maintainable if they are aligned- what do you think?

i sorta think that it might be nice to have the flag be browser instead of terminal... backwards compat has us a bit stuck though. gonna think on this- but would love to hear your thoughts!

Copy link
Contributor Author

@EverlastingBugstopper EverlastingBugstopper Oct 30, 2019

Choose a reason for hiding this comment

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

My preference would be that the default opens the browser and that the flag is browser. The reason I implemented it this way is because of the way things are today. That being said, I don't think we really need to worry too much about backwards compatibility wrt preview, but I could be wrong. As for the reason the flag is terminal and the preview function takes browser - it makes more sense semantically (to me) to test for browser == true than for terminal == false when trying to open the browser.

Copy link
Contributor

@ashleymichal ashleymichal Nov 1, 2019

Choose a reason for hiding this comment

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

should it be configurable via another method as well? there's the whole idea that if there's a flag for it, there should be a config setting for it, or that it should accept an environment variable.

Copy link
Contributor

@ashleymichal ashleymichal Nov 1, 2019

Choose a reason for hiding this comment

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

also, perhaps it isn't a no arg flag, but a bool value flag, like --browser=true, with a default?

@ashleygwilliams ashleygwilliams added this to the 1.6.0 milestone Oct 31, 2019
@EverlastingBugstopper
Copy link
Contributor Author

EverlastingBugstopper commented Nov 1, 2019

We've had a bunch of discussion around this that I will attempt to summarize here.

This feature is largely temporary until we deprecate responses being shown in the terminal for wrangler preview in favor of a new command wrangler curl.

--terminal doesn't quite map properly because if the flag is omitted, the output is opened in the browser, but also printed in the terminal.

@ashleymichal proposed changing the flag to --browser=false, which gets closer to what we want.

My favorite proposal is @xtuc's --headless as it is commonly used to describe this type of functionality, and also doesn't require the user to type =false.

@ashleymichal proposed that we also introduce a field in the manifest in addition to an environment variable to configure the default behavior so people can simply wrangler preview and it won't open the browser, even if they don't pass a flag. I'm personally not in favor of taking this approach as it will add cruft to people's manifests and their environment variables. We plan on deprecating this feature, and while we can deprecate the feature and the implementation at the same time, instead of simply asking people to type --headless, they will need to go through all of their projects and update their manifests. It also doesn't map well to the rest of what we allow to be configured in the manifests as it has nothing to do with deploying, and more to do with the personal preference of the individual developer.

We'd also like to get this in for the release candidate today.

TL;DR: I want to just change this PR from --terminal/-t to --headless

@ashleymichal
Copy link
Contributor

at the risk of sounding a bit rude, after all this time of this being a problem, why introduce a stop gap we intend to almost immediately deprecate, when we are actively working on the full-featured command that will replace it and the work is slated to complete this quarter?

@ashleygwilliams
Copy link
Contributor

i'm in favor of --headless (make the variable in the code the same, for maintenance sake), eventually deprecate for wrangler curl, and don't make it an env var because we will eventually deprecate

if we made it configurable it should be a global config not a per project config, and we dont have that beyond auth yet so i dont want to rush into introducing it

@EverlastingBugstopper
Copy link
Contributor Author

EverlastingBugstopper commented Nov 1, 2019

@ashleymichal I think that's a reasonable ask - and is why I'm opposed to introducing manifest changes. I think that this is a small enough change that will alleviate some pain for devs (including our CI running wrangler tests) until we get wrangler curl out.

I also think we'll want to keep --headless around for our own CI when we still want to run tests on preview without opening the browser.

@EverlastingBugstopper EverlastingBugstopper changed the base branch from avery/wrangler-curl to master November 1, 2019 16:11
@ashleymichal
Copy link
Contributor

ashleymichal commented Nov 1, 2019

I also think we'll want to keep --headless around for our own CI when we still want to run tests on preview without opening the browser.

does this imply that will we maintain long term support for this feature?

@EverlastingBugstopper
Copy link
Contributor Author

does this imply that will we maintain long term support for this feature?

I think so

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

give option to not open browser when previewing
3 participants