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.

Sign up for GitHub

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

Jump to bottom

Custom Miniflare CLI #633

Merged
merged 9 commits into from
Mar 22, 2022
Merged

Custom Miniflare CLI #633

merged 9 commits into from
Mar 22, 2022

Conversation

JacobMGEvans
Copy link
Contributor

Previously, Miniflare CLI was being ran for local through a child process which made passing options into it complicated for more complex inputs.

Solution: Create a custom CLI wrapper around Miniflare API
This allows us to tightly control the options that are passed to Miniflare.

  • Miniflare API config is more ergonomic and allows for far more complex inputs to be easily passed in.
  • Instantiation, server startup, and cleanup with the API is far more explicit and manageable.

Bugfix: Likely pathing to the current working directory only worked if ran in the directory with a Worker, the base directory is now an absolute path to the directory with a wrangler.toml

The current CLI is setup to be more compatible with how Wrangler 1 works, which is not optimal for Wrangler 2.

@changeset-bot
Copy link

changeset-bot bot commented Mar 17, 2022 •
edited
Loading

🦋 Changeset detected

Latest commit: 0d3b5ad

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
Copy link
Contributor

github-actions bot commented Mar 17, 2022 •
edited
Loading

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/2017790481/npm-package-wrangler-633

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

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

Or you can use npx with this latest build directly:

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

mrbbot
mrbbot reviewed Mar 17, 2022
View reviewed changes
Copy link
Contributor

@mrbbot mrbbot left a comment

Choose a reason for hiding this comment

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

Nice! 🎉 This looks really good! Added a few comments. 🙂

packages/wrangler/src/dev/local.tsx Outdated Show resolved Hide resolved
packages/wrangler/src/dev/local.tsx Show resolved Hide resolved
packages/wrangler/src/miniflare-cli/index.ts Show resolved Hide resolved
@petebacondarwin petebacondarwin force-pushed the custom-miniflare-cli branch 4 times, most recently from 2ba2587 to a1bdb72 Compare March 18, 2022 12:18
threepointone
threepointone reviewed Mar 20, 2022
View reviewed changes
packages/wrangler/scripts/bundle.ts Show resolved Hide resolved
packages/wrangler/src/dev/local.tsx Outdated
)
),
durableObjectsPersist: enableLocalPersistence,
cachePersist: !enableLocalPersistence,
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't this be just enableLocalPersistence?

Suggested change
cachePersist: !enableLocalPersistence,
cachePersist: enableLocalPersistence,

Copy link
Contributor

Choose a reason for hiding this comment

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

And yes again! I suspect this was a late night change 😵

threepointone
threepointone approved these changes Mar 21, 2022
View reviewed changes
Copy link
Contributor

@threepointone threepointone left a comment

Choose a reason for hiding this comment

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

Let's try to land this soon-ish so we can try it out. I'll give a stamp to unblock, but don't forget the package.json/cachePersist fixes

packages/wrangler/scripts/bundle.ts Show resolved Hide resolved
petebacondarwin and others added 8 commits March 21, 2022 09:53
@petebacondarwin @JacobMGEvans
refactor: create a custom CLI wrapper around Miniflare API
c44d244 
This allows us to tightly control the options that are passed to Miniflare.
The current CLI is setup to be more compatible with how Wrangler 1 works, which is not optimal for Wrangler 2.
@petebacondarwin @JacobMGEvans
refactor: use new Miniflare CLI for Local dev
45d9eae
@JacobMGEvans
Passing entry directory to spawn CWD in local file
5eeb6fb
@petebacondarwin @JacobMGEvans
pass absolute site path to miniflare
cf41d9f
@JacobMGEvans
PR suggestion: add sourceMap: true (for source mapped error messages)…
1d8af19 
… and logUnhandledRejections: true (to catch unhandled rejections instead of crashing) here
@petebacondarwin @JacobMGEvans
fix cachePersist Miniflare setting
7a3d2af
@petebacondarwin @JacobMGEvans
fix: ensure asset keys are relative to the project root
4766c27 
Previously, asset file paths were computed relative to the current working
directory, even if we had used `-c` to run Wrangler on a project in a different
directory to the current one.

Now, assets file paths are computed relative to the "project root", which is
either the directory containing the wrangler.toml or the current working directory
if there is no config specified.
@JacobMGEvans
PR Review changes applied
3fe320a
@JacobMGEvans
The pathing to Site assetDirectory needs to be absolute and relative
0d3b5ad 
added a join to the helper that is passed into Dev for asset paths.
@petebacondarwin petebacondarwin merged commit 8bd7151 into main Mar 22, 2022
@petebacondarwin petebacondarwin deleted the custom-miniflare-cli branch March 22, 2022 12:13
@github-actions github-actions bot mentioned this pull request Mar 22, 2022
Merged
@petebacondarwin petebacondarwin mentioned this pull request Mar 22, 2022
Closed
threepointone added a commit that referenced this pull request Mar 23, 2022
@threepointone
fix: resolve non-js modules correctly in local mode
961f620 
In #633, we missed passing a cwd to the process that runs the miniflare cli. This broke how miniflare resolves modules, and led back to the dreaded "path should be a `path.relative()`d string" error. The fix is to simply pass the cwd to the `spawn` call.
threepointone added a commit that referenced this pull request Mar 23, 2022
@threepointone
fix: resolve non-js modules correctly in local mode
aadfbf4 
In #633, we missed passing a cwd to the process that runs the miniflare cli. This broke how miniflare resolves modules, and led back to the dreaded "path should be a `path.relative()`d string" error. The fix is to simply pass the cwd to the `spawn` call.

Test plan:

```
cd packages/wrangler
npm run build
cd ../workers-chat-demo
npx wrangler dev --local
```
@threepointone threepointone mentioned this pull request Mar 23, 2022
Merged
threepointone added a commit that referenced this pull request Mar 23, 2022
@threepointone
fix: resolve non-js modules correctly in local mode (#675)
e88a54e 
In #633, we missed passing a cwd to the process that runs the miniflare cli. This broke how miniflare resolves modules, and led back to the dreaded "path should be a `path.relative()`d string" error. The fix is to simply pass the cwd to the `spawn` call.

Test plan:

```
cd packages/wrangler
npm run build
cd ../workers-chat-demo
npx wrangler dev --local
```
mrbbot added a commit that referenced this pull request May 11, 2023
@mrbbot
chore: remove unused files from published package
98ae50e 
Prevents the following being included in the npm tarball:

- `src`: we may want to ship this if we shipped source maps, so
  users could click on internal lines in stack traces, but we don't
  ship source maps to npm, so these aren't needed.
- `miniflare-config-stubs`: these aren't used as of #633
  (https://github.com/cloudflare/workers-sdk/pull/633/files#diff-dd2c9d6fc577a3da1b7cf0b6b4a0705a1c71eb5d99c372d5bdda99476362f975L97-L102)
- `vendor`: this directory doesn't actually exist in the `wrangler`
   package directory, there is a root `vendor` directory, but files
   from that are copied to `wrangler-dist` during build

Closes #3013
@mrbbot mrbbot mentioned this pull request May 11, 2023
Merged
2 tasks
mrbbot added a commit that referenced this pull request May 12, 2023
@mrbbot
chore: remove unused files from published package (#3191)
8164c52 
Prevents the following being included in the npm tarball:

- `src`: we may want to ship this if we shipped source maps, so
  users could click on internal lines in stack traces, but we don't
  ship source maps to npm, so these aren't needed.
- `miniflare-config-stubs`: these aren't used as of #633
  (https://github.com/cloudflare/workers-sdk/pull/633/files#diff-dd2c9d6fc577a3da1b7cf0b6b4a0705a1c71eb5d99c372d5bdda99476362f975L97-L102)
- `vendor`: this directory doesn't actually exist in the `wrangler`
   package directory, there is a root `vendor` directory, but files
   from that are copied to `wrangler-dist` during build

Closes #3013
penalosa pushed a commit that referenced this pull request May 17, 2023
@mrbbot @penalosa
chore: remove unused files from published package (#3191)
a3a3490 
Prevents the following being included in the npm tarball:

- `src`: we may want to ship this if we shipped source maps, so
  users could click on internal lines in stack traces, but we don't
  ship source maps to npm, so these aren't needed.
- `miniflare-config-stubs`: these aren't used as of #633
  (https://github.com/cloudflare/workers-sdk/pull/633/files#diff-dd2c9d6fc577a3da1b7cf0b6b4a0705a1c71eb5d99c372d5bdda99476362f975L97-L102)
- `vendor`: this directory doesn't actually exist in the `wrangler`
   package directory, there is a root `vendor` directory, but files
   from that are copied to `wrangler-dist` during build

Closes #3013
petebacondarwin pushed a commit that referenced this pull request May 17, 2023
@mrbbot @petebacondarwin
chore: remove unused files from published package (#3191)
24e1607 
Prevents the following being included in the npm tarball:

- `src`: we may want to ship this if we shipped source maps, so
  users could click on internal lines in stack traces, but we don't
  ship source maps to npm, so these aren't needed.
- `miniflare-config-stubs`: these aren't used as of #633
  (https://github.com/cloudflare/workers-sdk/pull/633/files#diff-dd2c9d6fc577a3da1b7cf0b6b4a0705a1c71eb5d99c372d5bdda99476362f975L97-L102)
- `vendor`: this directory doesn't actually exist in the `wrangler`
   package directory, there is a root `vendor` directory, but files
   from that are copied to `wrangler-dist` during build

Closes #3013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mrbbot mrbbot mrbbot left review comments

@threepointone threepointone threepointone approved these changes

@petebacondarwin petebacondarwin Awaiting requested review from petebacondarwin

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@JacobMGEvans @petebacondarwin @threepointone @mrbbot