feat: export a dev function that folks can use in their own scripts to invoke wrangler dev #1350

rozenmd · 2022-06-24T15:19:48Z

Added a warning:

Things for follow-up releases:

unstable_dev should return an object so you can just do:

const worker = await wrangler.unstable_dev(...)
const resp =  await worker.fetch()

we should make miniflare notify wrangler that it's ready for requests (the remote dev worker supports this already)
(for future us to deal with): split wrangler into @wrangler/core and @wrangler/renderer (or something) so prop-drilling to reach the core isn't necessary

Follow-up issues for tracking:

changeset-bot · 2022-06-24T15:19:51Z

🦋 Changeset detected

Latest commit: 645d19e

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
wrangler	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

github-actions · 2022-06-24T15:20:52Z

A wrangler prerelease is available for testing. You can install this latest build in your project with:

npm install --save-dev https://prerelease-registry.developers.workers.dev/runs/2596257088/npm-package-wrangler-1350

You can reference the automatically updated head of this PR with:

npm install --save-dev https://prerelease-registry.developers.workers.dev/prs/1350/npm-package-wrangler-1350

Or you can use npx with this latest build directly:

npx https://prerelease-registry.developers.workers.dev/runs/2596257088/npm-package-wrangler-1350 dev path/to/script.js

JacobMGEvans · 2022-06-24T15:24:16Z

packages/wrangler/src/cli.ts

-  const exitCode = (e instanceof FatalError && e.code) || 1;
-  process.exit(exitCode);
-});
+if (require.main) {


Until we have a architecture or abstraction for the API layer I think it would be good to have simple comments expressing what/why things like this are being implemented, i.e. the code looks like a CLI but there is library API package logic sprinkled seemingly randomly

packages/wrangler/src/dev.tsx

petebacondarwin

We should not have a package-lock.json in any of the subfolders of the monorepo,
Can you delete the one in wrangler-dev-api-app?

threepointone

This is looking pretty great. Dropped some notes, would like to bikeshed the isApi param for a bit

fixtures/wrangler-dev-api-app/package.json

threepointone · 2022-06-29T09:49:13Z

package-lock.json

@@ -503,6 +503,10 @@
 		"fixtures/workers-chat-demo": {
 			"version": "1.0.0"
 		},
+		"fixtures/wrangler-dev-api-app": {


the changes in this file are concerning, seems like a lot and I'm not sure why

@rozenmd - I see you managed to reduce the churn. What did you do to make that happen?

good call out @threepointone - seems like the fixture's own package-lock made this one funky. Pulled in the one from master and re-ran npm i to fix it up.

packages/wrangler/src/dev/dev.tsx

threepointone · 2022-06-29T09:56:34Z

Oh, please add a changeset!

petebacondarwin

I added lots of suggestions. Nothing blocking this, since you are actively iterating on this.
But do take a look and see what you think.

petebacondarwin · 2022-06-29T10:17:48Z

fixtures/wrangler-dev-api-app/src/wrangler-dev.mjs

+import wrangler from "wrangler";
+
+//since the script is invoked from the directory above, need to specify index.js is in src/
+await wrangler.unstable_dev("src/index.js");


If you wanted to be super resilient here you could do something much more complicated:

import * as path from 'path'; import * as url from 'url'; const __dirname = url.fileURLToPath(new URL('.', import.meta.url)); await wrangler.unstable_dev(path.resolve(__dirname, "./index.js"));

fixtures/wrangler-dev-api-app/wrangler.toml

petebacondarwin · 2022-06-29T10:22:58Z

package-lock.json

@@ -503,6 +503,10 @@
 		"fixtures/workers-chat-demo": {
 			"version": "1.0.0"
 		},
+		"fixtures/wrangler-dev-api-app": {


@rozenmd - I see you managed to reduce the churn. What did you do to make that happen?

petebacondarwin · 2022-06-29T10:24:20Z

packages/wrangler/src/api/dev.ts

+	logger.warn(
+		`unstable_dev() is experimental\nunstable_dev()'s behaviour will likely change in future releases`
+	);
+	logger.loggerLevel = "error";


Out of curiosity, why are you overriding the loggerLevel here?

The logger gives irrelevant information otherwise (for API users): ⎔ Starting a local server...

As a follow up: For API users they should be able to provide their own Logger instance, which they can then implement as they see fit.

fixtures/wrangler-dev-api-app/package.json

petebacondarwin · 2022-06-29T10:33:38Z

packages/wrangler/package.json

@@ -7,6 +7,7 @@
 		"wrangler": "./bin/wrangler.js",
 		"wrangler2": "./bin/wrangler.js"
 	},
+	"main": "wrangler-dist/cli.js",


Rather than conflating the two, I wonder if we should actually have two generated entry-points for wrangler?

wrangler-dist/index.js wrangler-dist/cli.js

The first would only contain the unstable_dev() function (for now).
The second would only contain the binary that is run when Wrangler is used as a CLI.

Then "main" would just point at the index.js.

Actually, we could even bundle the API entry-point as index.mjs and then have a "module" property here for futureproofing.

Can this be an immediate follow-up task @petebacondarwin ?

👍 all my comments can be follow-ups

Does sound like additional scope to me.

packages/wrangler/src/dev/remote.tsx

packages/wrangler/src/miniflare-cli/index.ts

petebacondarwin · 2022-06-29T10:40:59Z

packages/wrangler/src/dev/local.tsx

+			local.current = spawn("node", localServerOptions, {
+				cwd: path.dirname(scriptPath),
+			});
+			//TODO: instead of being lucky with spawn's timing, have miniflare-cli notify wrangler that it's ready in packages/wrangler/src/miniflare-cli/index.ts, after the mf.startScheduler promise resolves


packages/wrangler/src/api/dev.ts

petebacondarwin · 2022-06-29T10:54:46Z

packages/wrangler/src/dev.tsx

-		await waitUntilExit();
+		const devReactElement = render(await getDevReactElement(config));
+		rerender = devReactElement.rerender;
+		return { devReactElement, watcher };


How about we return our own waitForExit function here that does the watcher.close() for us?
E.g.

async function waitForExit() { try { await devReactElement.waitUntilExit(); } finally { watcher?.close(); } } return { waitForExit };

Then in the caller we don't need to know about the watcher:

export async function devHandler(args: ArgumentsCamelCase<DevArgs>) { const { waitForExit } = await startDev(args); await waitUntilExit(); }

petebacondarwin · 2022-06-29T10:58:08Z

Last comment! I am not convinced this should have a changeset until it is in a place where we want people to start playing with it. That should be very soon, but not as a result of this PR landing, IMO.

rozenmd · 2022-06-29T14:24:13Z

I think we do want people playing with it @petebacondarwin, hence the changeset. Any scripts they write will break (slightly) in future releases (especially after we move to worker.fetch instead of undici.fetch), but that's what the unstable_ prefix and scary warning are for.

JacobMGEvans · 2022-06-29T16:19:28Z

I think we do want people playing with it @petebacondarwin, hence the changeset. Any scripts they write will break (slightly) in future releases (especially after we move to worker.fetch instead of undici.fetch), but that's what the unstable_ prefix and scary warning are for.

Any changes to the API would require us to update the tests which we can take snippets from then added to future changesets and referenced easily for updating their code.

…o invoke wrangler dev Update packages/wrangler/src/dev/remote.tsx Co-authored-by: Pete Bacon Darwin <[email protected]> address PR feedback

threepointone

approving, but a few notes for before you land it. let's goooo

.changeset/little-horses-perform.md

fixtures/wrangler-dev-api-app/wrangler.toml

threepointone · 2022-07-01T09:31:07Z

packages/wrangler/src/api/dev.ts

+	);
+
+	return new Promise<{ stop: () => void }>((resolve) => {
+		//lmao


lmao is now part of the spec

congrats on getting this to work, let's land it and see if we can simplify it in another pr

threepointone · 2022-07-01T09:31:43Z

packages/wrangler/src/api/dev.ts

+	);
+
+	return new Promise<{ stop: () => void }>((resolve) => {
+		//lmao


lmao is now part of the spec

congrats on getting this to work, let's land it and see if we can simplify it in another pr

packages/wrangler/src/miniflare-cli/index.ts

petebacondarwin · 2022-07-01T10:15:15Z

fixtures/wrangler-dev-api-app/src/wrangler-dev.mjs

+
+console.log("Invoked worker: ", text);
+
+await worker.stop();


petebacondarwin · 2022-07-01T10:17:15Z

fixtures/wrangler-dev-api-app/src/wrangler-dev.mjs

+//since the script is invoked from the directory above, need to specify index.js is in src/
+const worker = await wrangler.unstable_dev("src/index.js");
+
+const resp = await undici.fetch("http://localhost:8787/");


It might be nice to hide the undici library, and also the underlying port, side of this and provide:

worker.fetch()

This method could take a path (or even a full URL if you wanted to simulate a custom route). This would also mitigate the case where port 8787 was not available and we ended up jumping on a different port, which I think we can do?

petebacondarwin · 2022-07-01T10:22:52Z

fixtures/wrangler-dev-api-app/package.json

+	"version": "1.0.0",
+	"description": "",
+	"scripts": {
+		"dev": "node --no-warnings ./src/wrangler-dev.mjs"


Now we have worker.stop() we could easily add a unit test here that is very similar to wrangler-dev.mjs.

Something along the lines of:

import undici from "undici"; import wrangler from "wrangler"; describe("worker", () => { it("should invoke the worker and exit", async () => { //since the script is invoked from the directory above, need to specify index.js is in src/ const worker = await wrangler.unstable_dev("src/index.js"); const resp = await undici.fetch("http://localhost:8787/"); const text = await resp.text(); expect(text).toMatchInlineSnapshot( `"http://localhost:8787/ Fri Jul 01 2022 11:41:21 GMT+0100 (British Summer Time)"` ); await worker.stop(); }); });

rozenmd · 2022-07-01T11:16:46Z

packages/wrangler/src/dev/local.tsx

@@ -254,7 +254,11 @@ function useLocalWorker({
 				cwd: path.dirname(scriptPath),
 			});
 			//TODO: instead of being lucky with spawn's timing, have miniflare-cli notify wrangler that it's ready in packages/wrangler/src/miniflare-cli/index.ts, after the mf.startScheduler promise resolves
-			onReady && onReady();
+			if (onReady) {
+				await new Promise((resolve) => setTimeout(resolve, 200));


this isn't necessary when running a script within the mono-repo, but when you run the preview build via npx, it's needed 😢

rozenmd self-assigned this Jun 24, 2022

JacobMGEvans reviewed Jun 24, 2022

View reviewed changes

packages/wrangler/src/dev.tsx Outdated Show resolved Hide resolved

rozenmd force-pushed the dev-as-an-api branch 4 times, most recently from 8b255a0 to 16226f6 Compare June 29, 2022 09:41

rozenmd marked this pull request as ready for review June 29, 2022 09:43

rozenmd requested review from threepointone and petebacondarwin as code owners June 29, 2022 09:43

petebacondarwin reviewed Jun 29, 2022

View reviewed changes

threepointone reviewed Jun 29, 2022

View reviewed changes

rozenmd force-pushed the dev-as-an-api branch from c303068 to 67ec72a Compare June 29, 2022 10:42

petebacondarwin approved these changes Jun 29, 2022

View reviewed changes

rozenmd force-pushed the dev-as-an-api branch from 12919a2 to fbba322 Compare June 29, 2022 10:51

petebacondarwin reviewed Jun 29, 2022

View reviewed changes

rozenmd force-pushed the dev-as-an-api branch from 8dc5111 to 845b655 Compare June 30, 2022 12:52

rozenmd changed the base branch from main to deprecate-preview June 30, 2022 13:14

Base automatically changed from deprecate-preview to main June 30, 2022 14:37

rozenmd added 3 commits June 30, 2022 17:41

feat: export a dev function that folks can use in their own scripts t…

4a5dea2

…o invoke wrangler dev Update packages/wrangler/src/dev/remote.tsx Co-authored-by: Pete Bacon Darwin <[email protected]> address PR feedback

replace isApi with logLevel, inspect, and showInteractiveDevSession

5705ad0

try to figure out a stop function

064ad39

rozenmd force-pushed the dev-as-an-api branch from 845b655 to 064ad39 Compare June 30, 2022 15:41

return a stop function

fff6803

threepointone approved these changes Jul 1, 2022

View reviewed changes

address PR feedback

49fa8c1

petebacondarwin reviewed Jul 1, 2022

View reviewed changes

actually shutdown on stop, and wait until we're ready

645d19e

rozenmd commented Jul 1, 2022

View reviewed changes

rozenmd merged commit dee034b into main Jul 1, 2022

rozenmd deleted the dev-as-an-api branch July 1, 2022 11:24

rozenmd mentioned this pull request Jul 1, 2022

Enhancement: API should have a separate entrypoint #1382

Closed

github-actions bot mentioned this pull request Jul 1, 2022

Version Packages #1336

Merged

This was referenced Jul 1, 2022

Enhancement: API/unstable_dev should expose a fetch() function #1383

Closed

Enhancement: API/unstable_dev needs tests #1384

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: export a dev function that folks can use in their own scripts to invoke wrangler dev #1350

feat: export a dev function that folks can use in their own scripts to invoke wrangler dev #1350

rozenmd commented Jun 24, 2022 •

edited

Loading

changeset-bot bot commented Jun 24, 2022 •

edited

Loading

github-actions bot commented Jun 24, 2022 •

edited

Loading

JacobMGEvans Jun 24, 2022

petebacondarwin left a comment

threepointone left a comment

threepointone Jun 29, 2022

petebacondarwin Jun 29, 2022

rozenmd Jun 29, 2022

threepointone commented Jun 29, 2022

petebacondarwin left a comment

petebacondarwin Jun 29, 2022

petebacondarwin Jun 29, 2022

petebacondarwin Jun 29, 2022

rozenmd Jun 29, 2022

petebacondarwin Jun 29, 2022

petebacondarwin Jun 29, 2022

petebacondarwin Jun 29, 2022

rozenmd Jun 29, 2022

petebacondarwin Jun 29, 2022

JacobMGEvans Jun 29, 2022

petebacondarwin Jun 29, 2022

petebacondarwin Jun 29, 2022 •

edited

Loading

petebacondarwin commented Jun 29, 2022

rozenmd commented Jun 29, 2022 •

edited

Loading

JacobMGEvans commented Jun 29, 2022

threepointone left a comment

threepointone Jul 1, 2022

threepointone Jul 1, 2022

petebacondarwin Jul 1, 2022

petebacondarwin Jul 1, 2022

petebacondarwin Jul 1, 2022

petebacondarwin Jul 1, 2022

rozenmd Jul 1, 2022

feat: export a dev function that folks can use in their own scripts to invoke wrangler dev #1350

feat: export a dev function that folks can use in their own scripts to invoke wrangler dev #1350

Conversation

rozenmd commented Jun 24, 2022 • edited Loading

changeset-bot bot commented Jun 24, 2022 • edited Loading

🦋 Changeset detected

github-actions bot commented Jun 24, 2022 • edited Loading

Choose a reason for hiding this comment

petebacondarwin left a comment

Choose a reason for hiding this comment

threepointone left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

threepointone commented Jun 29, 2022

petebacondarwin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petebacondarwin Jun 29, 2022 • edited Loading

Choose a reason for hiding this comment

petebacondarwin commented Jun 29, 2022

rozenmd commented Jun 29, 2022 • edited Loading

JacobMGEvans commented Jun 29, 2022

threepointone left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rozenmd commented Jun 24, 2022 •

edited

Loading

changeset-bot bot commented Jun 24, 2022 •

edited

Loading

github-actions bot commented Jun 24, 2022 •

edited

Loading

petebacondarwin Jun 29, 2022 •

edited

Loading

rozenmd commented Jun 29, 2022 •

edited

Loading