Added URL support for preview command

[danzlarkin]

Hey team,

Thanks for all the great work you and everyone else has done so far!

I have added support a custom URL to be passed to the preview command.

When either a URL is passed using the flag -u or --url to the preview command the browser will open to the URL specified.

Example usage:

$ wrangler preview --url https://example.com/endpoint?id=random

In the case where --headless is also passed, the server will output the response of the request to the URL.

When this flag is not specified the application will continue to use the default https://example.com.

The protocol specified in the URL can be either HTTP or HTTPS.

In the case that neither protocol are used, or the specified URL is not a valid URL, the command will fallback to using the default as specified above.

I hope you can review and implement this ASAP as I know a number of developers would love this feature!

Thanks heaps

[danzlarkin]

A side note relevant to this feature.

It seems that currently, the CloudflareWorkers preview server automatically appends a forward-slash to the final component of the __ew_fiddle_preview cookie (the domain).

I guess this currently occurs as the server currently expects a domain rather than a domain + pathname + query.

For example, if the final component of the cookie value is example.com/?query=10 the server will process it as example.com/?query=10/.

This doesn't break anything, but it would good if this trailing forward-slash was removed when not needed (i.e. when the final section of the URL is either a pathname or query).

A very minor change required, but obviously something which I cannot do myself haha.

Thanks again

[danzlarkin]

Added tests now also!

[EverlastingBugstopper]

Hey thanks for this PR! I believe it fixes #351.

I left a few comments about the approach that I hope will help move this along!

We're also working on a new feature: wrangler dev which will likely replace wrangler preview entirely - instead of opening the browser, it will spin up a local dev server that you can then send requests to with any http client you choose. We're planning on doing an alpha release of Wrangler that includes wrangler dev by the end of the month.

EDIT: you'll also need to run cargo fmt in order for our CI checks to let this through

[danzlarkin]

Great to hear about the dev feature, this will be very useful for sure!

I guess this will just be a form of proxy around the existing endpoint (https://00000000000000000000000000000000.cloudflareworkers.com) which sends the necessary cookie?

I was heavily involved in the development of Cloudworker (https://github.com/dollarshaveclub/cloudworker)` long back before wrangler came out and it would be great to see the workflow get much closer to this.

In some ways my own library Restt-CLI (https://github.com/resttjs/restt-cli) was a very early precursor to wrangler well before workers.dev was even setup.

It's nice to be involved with improving workers where possible as I have since way back!

Anyways, I've patched what was mentioned above in the previous commit.

I have also added support now for the --watch flag to work correctly.

If the --watch flag is on then each request will be logged using the client_request function added in the preview command when running with --headless.

client_request is responsible for making a request using the reqwest client using the method, body and request_payload.

All changes are working correctly now as needed.

I hope you can review this and integrate it to the core as soon as possible.

Thanks again for all the great work!

[danzlarkin]

@EverlastingBugstopper any idea when this will be reviewed and approved?

[danzlarkin]

@EverlastingBugstopper - any reason this pull request is taking so long to be approved?

[EverlastingBugstopper]

Hi @larkin-nz - I was on caretaker leave the past couple of days, taking some time to look at this now. Thanks for your patience.

[EverlastingBugstopper]

this looks really good - just a few behavior changes i'd like to see and then we can get this merged - you can ignore anything that says nit i won't block on those

[danzlarkin]

Hey @EverlastingBugstopper - I've added up those updates!

All is working as expected on Ubuntu and Windows locally, but obviously the Windows test is still failing as referenced in #1012

I hope you're able to merge this request now!

Thanks

[EverlastingBugstopper]

this lgtm - i'm going to wait to merge until we get our CI figured out so we don't get caught up with some weird rebases/unexpected errors and then this should be good to go! Thanks @larkin-nz for your contribution :)

[danzlarkin]

Excellent, thank you @EverlastingBugstopper!

Hope you work out the issues soon.

Thanks for maintaining the project!

[danzlarkin]

Just added a quick patch to the logic.

Previously on startup, both the client and the user's web browser would perform a request to the URL.

This should not be the case, the request from client should only be made with --headless

[EverlastingBugstopper]

Hey @larkin-nz - that's actually expected behavior. We still want the output of the request to show up in the terminal even if the browser is opened 😄. If you could revert to right before that change that would be 💯

[danzlarkin]

This behaviour was already removed from by the first commit I made as this pull request because we don't want duplicate requests occuring. I think it would be better to pipe the output from the browser to the terminal using a socket if we do output it. Currently it will make two requests and this can be painful for testing things.

[EverlastingBugstopper]

@ashleymichal do you have thoughts on removing the output when wrangler preview is not run with the --headless argument? I'm hesitant to change it because it changes the default behavior that people expect. I'm especially worried about folks on the Windows Linux Subsystem who haven't configured their browser to open and are used to running wrangler preview and the output being there in their terminal.

@larkin-nz I'm interested in how duplicate requests makes it painful to test things - I agree it's not the most efficient but if you're testing can you just ignore the output in the terminal if you're not using --headless?

[ashleymichal]

i can see how duplicate output can make testing difficult, though i had assumed (perhaps naively) that folks running automated tests would opt in to the --headless flag.

i'd be inclined to at least include instructions for those in the Windows Linux subsystem camp to take action (i.e. set their browser, or use the --headless flag; if there's a better way we should be encouraging users to use that rather than supporting iffy behavior. That being said, it could break folks' CI or development workflows and to me that suggests "breaking change", which deserves the rollout procedure that goes with such changes, i.e. deprecation warnings etc.

TL;DR - I'm not opposed to making this change, but I think we should be courteous in how we release it.

[danzlarkin]

@ashleymichal and @EverlastingBugstopper -- I can understand your logic here.

I guess an example of where this is a problem is a case where I have been making a CRON style task scheduler within Workers and KV using Streams.

When there are duplicate requests then the service ends up having multiple streams glitching out on different sessions while writing to KV at the same time due to the slow speeds of KV writing and reading. This can lead to a task in the schedule to be ran twice which is problematic in some cases.

Obviously this is a fairly specific case and is complex, but it still highlights the point of concern. Duplicate requests can cause a number of issue with testing things such as Stripe payments (Stripe will decline duplicates).

I guess where my logic sits is that if the --headless flag is used then the request should be performed from the terminal. In the case where it is not it should be performed from the browser as that is the whole point of the preview page.

Headless would be for testing a request, where as standard preview would be exactly that, a interactive preview.

Would it not be easier to just change the way the preview site works so we can pipe back the response from there via a WebSocket so there are no duplicates. Maybe we can already do this without needing to change the preview site? I am unsure.

Let me know what you think!

[EverlastingBugstopper]

@larkin-nz if you have time to implement piping output to the terminal from the browser by all means go ahead!

I don't think we can merge this as is though due to the backwards compatibility issues previously discussed.

We are also planning on releasing an alpha version of wrangler dev soon™️ which may satisfy your testing needs in a more ergonomic manner.

[danzlarkin]

@EverlastingBugstopper - I'll revert the changes to how it was so you can merge this into the main branch. I'll then work on a new new pull later next week when I get the chance. Cheers

…

On Thu, 13 Feb 2020, 06:24 Avery Harnish, ***@***.***> wrote: @larkin-nz <https://github.com/larkin-nz> if you have time to implement piping output to the terminal from the browser by all means go ahead! I don't think we can merge this as is though due to the backwards compatibility issues previously discussed. We are also planning on releasing a beta version of wrangler dev soon™️ which may satisfy your testing needs in a more ergonomic manner. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1001>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AFM63D7LOY55JNRJZNINZM3RCQWDHANCNFSM4KHATPRA> .

[EverlastingBugstopper]

@larkin-nz - in the meantime, we've released the beta! If you want to try it out you can install with npm i -g @cloudflare/wrangler@beta and check out the testing instructions.

[danzlarkin]

@EverlastingBugstopper - great work will check this out soon! About to jump on another international flight but I'll add up the changes once I'm settled down again in the next couple days ☺️

[danzlarkin]

@EverlastingBugstopper - sorry, have had a family emergency and haven't been online. Will aim to push up this stuff early next week. Cheers

[EverlastingBugstopper]

Hey no worries at all 😄

[danzlarkin]

Hey @EverlastingBugstopper & @ashleymichal

Sorry for the delays (my brother passed away).

I've reverted the logic to be correct for those requests.

I'll try to add some logic as discussed above with WebSockets at a later date.

For now, this is all working as expected so this request can be merged!

Keep up the good work

[EverlastingBugstopper]

looking good - will need to run latest cargo fmt (seems like you picked up some changes in unrelated code), address the comment I have about printing with Workers Sites, resolve conflicts and we should be good to go. sorry this has been such a long running process 😄

[danzlarkin]

@EverlastingBugstopper

All done now -- ready to be merged!

Cheers

[EverlastingBugstopper]

lgtm!