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

Module upload format + custom build commands + durable object support #1677

Merged
merged 5 commits into from
Feb 25, 2021

Conversation

xortive
Copy link
Contributor

@xortive xortive commented Dec 11, 2020

  • remove linter and add custom build options as part of JS TargetType

JS projects with current config options will continue to work the same.
if a [builder-config] section is added, a custom build command can be configured. Wrangler will handle watching for changes and running the build command for the user.

  • support uploading in module format

when the script-format field in [builder-config] is set to modules, we should upload using the new modules format, uploading each file in builder-config.output_dir as a module.

The main_module is determined by looking at the module field in package.json. this should point to the bundled module in the output directory.

for service-worker builds, the behavior is the same as always: the main field in package.json is what will be uploaded, so this should be the output of your build tool.

  • support creating/implementing durable object namespaces
  • support binding/using durable object namespaces

This PR closes a lot of tickets with the same general theme (I hit xyz build edge case and I need a new feature to fix it/I want to use newer webpack version/I don't want to use webpack) that all come down to the limitations of wrangler's current build setup. Custom builds will allow users to setup their own build step, which wrangler will upload the results of unmodified to the API.

closes #206
closes #820
closes #954
partially addresses #1102
closes #1397 (just make custom build script)
closes #1550
closes #1557
closes #1558

@xortive xortive requested a review from a team as a code owner December 11, 2020 19:22
@xortive xortive force-pushed the malonso/modules-do branch 4 times, most recently from 4a42cb4 to cc82568 Compare December 11, 2020 21:14
@xortive xortive requested a review from ags799 December 11, 2020 22:54
@xortive xortive force-pushed the malonso/modules-do branch 7 times, most recently from 441c591 to 34e3164 Compare December 17, 2020 17:14
@kentonv
Copy link
Member

kentonv commented Dec 28, 2020

Is there a design doc somewhere? I don't know this codebase well enough to review the code but I'm interested to understand how this will end up looking from the user's perspective.

@tillkruss
Copy link

Is there a design doc somewhere? I don't know this codebase well enough to review the code but I'm interested to understand how this will end up looking from the user's perspective.

And may I request an example of how to use a simple example like the Counter in the folder structure of a Worker Site?

@koeninger
Copy link
Contributor

@kentonv the only design doc was that google doc linked from our meeting on dec 9.

I believe Matt's plan was to share an example wrangler template, which should hopefully make things clearer. It'll be at least a couple weeks before there's more progress on this PR though.

@xtuc
Copy link
Member

xtuc commented Dec 29, 2020

@koeninger would you mind sharing the document? i'd like to take a look too

@ags799
Copy link
Contributor

ags799 commented Dec 29, 2020 via email

}

fn build_dir() -> PathBuf {
default_warning("build dir", BUILD_DIR);
Copy link
Contributor

@koeninger koeninger Jan 5, 2021

Choose a reason for hiding this comment

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

It's a little confusing to warn on build dir and src dir when they aren't actually used if upload_format is set to service-worker, right?

edit - looks like src_dir is used, but only for watch? It's still confusing that I can silence the warning by putting a nonexistent directory in there, and publish fine.

Copy link
Contributor Author

@xortive xortive Jan 11, 2021

Choose a reason for hiding this comment

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

I'll address this when I address @ags799 concern below about having src/build dir be ./, we don't want to allow that or allow nonexistent directories.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I ended up removing the default warning. I'll add a separate validation for the ./ and nonexistent directory cases

src/build/mod.rs Show resolved Hide resolved
src/build/mod.rs Outdated Show resolved Hide resolved
src/commands/publish.rs Outdated Show resolved Hide resolved
src/settings/toml/builder.rs Outdated Show resolved Hide resolved
@@ -0,0 +1,66 @@
use std::env;
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be really helpful to see an example TOML with the new additions.

Copy link
Contributor

Choose a reason for hiding this comment

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

The new additions would look something like this for a durable object

[builder_config]
build_dir = "./build"
src_dir = "./"
upload_format = "modules"

[durable_objects]
implements = [
  { class_name = "Counter" }
]

or for a regular service worker just

[builder_config]
build_command = "./dummy-build.sh"
build_dir = "./my-build-dir"
src_dir = "./does-not-exist"
upload_format = "service-worker"

Copy link
Contributor

@ObsidianMinor ObsidianMinor Jan 21, 2021

Choose a reason for hiding this comment

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

Lots of underscores and duplicate info... Perhaps it could be streamlined?

[build]
cwd = "."
command = "sh ./dummy-build.sh"
output = "./build"
format = "modules"

[durable_objects] # can't really avoid that one
implements = [{ class = "Counter" }]

I say this because toml is pretty flexible with how you can write out tables and while you may write a table like that, I may write it like

build.cwd = "."
build.command = "sh ./dummy-build.sh"
build.output = "./build"
build.format = "modules"

durable_objects.implements = [{ class = "Counter" }]

With the long names you get

builder_config.build_dir = "."
builder_config.build_command = "sh ./dummy-build.sh"
builder_config.src_dir = "./build"
builder_config.upload_format = "modules"

durable_objects.implements = [{ class_name = "Counter" }]

Toml accepts both but shorter names look better with the second format.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tend to prefer verbosity when it eliminates confusion, but I agree on build_command -> command.
I'll defer to @greg-mckeon on the rest

Copy link
Contributor

Choose a reason for hiding this comment

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

I like command, and I think that src_dir should be working_dir. And I mentioned earlier that build_dir should be upload_dir.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd personally want it to be configurable with reasonable defaults. So you can choose a different directory to watch, but it uses the build command's cwd by default, or perhaps just the root folder (where wrangler.toml is).

Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we talking about configuring cwd?
Wrangler won't work anywhere but the directory with wrangler.toml
If someone needs a different cwd for their build, they can change to it and back in their build script.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for explaining @xortive. I think src_dir is good.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah :/ I made the change to watch_dir already. It's easy to put it back, and I'm completely indifferent to which one we pick

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh that's fine idc either way.

src/upload/form/modules_worker.rs Outdated Show resolved Hide resolved
src/upload/form/modules_worker.rs Outdated Show resolved Hide resolved
src/upload/form/modules_worker.rs Show resolved Hide resolved
src/upload/package.rs Show resolved Hide resolved
src/watch/mod.rs Outdated Show resolved Hide resolved
@ags799
Copy link
Contributor

ags799 commented Jan 11, 2021

Does it make sense to support a modules project without a build step? If I had a modules project with one source file at ./index.mjs:

  1. I would need to configure my src_dir and build_dir to be './'.
  2. Wrangler would then try to upload all files in './' as modules, and fail on something like .gitignore because it can't parse the extension.
  3. I would move index.mjs to a new src directory and update src_dir and build_dir.

I think we can solve this by moving upload_format from builder_config to top-level configuration. So we don't tie module support to having a build step with source and build directories.

I'll make a line comment about the .gitignore failure.

@koeninger
Copy link
Contributor

I think we can solve this by moving upload_format from builder_config to top-level configuration. So we don't tie module support to having a build step with source and build directories.

We still need to know where to look for files to upload, right? Having to specify a subdirectory seems about as inconvenient / confusing as having to specify ignore patterns. I guess we could special case single file modules using the filename from package.json module, but that seems hacky

@xortive
Copy link
Contributor Author

xortive commented Feb 25, 2021

going to open a new PR with just the durable object changes. will be merging the modules + toml-edit changes into a release branch for the rc

@xortive xortive changed the base branch from master to malonso/modules-rc February 25, 2021 14:34
@xortive xortive merged commit e5b8d5b into malonso/modules-rc Feb 25, 2021
@delete-merged-branch delete-merged-branch bot deleted the malonso/modules-do branch February 25, 2021 14:35
threepointone added a commit to cloudflare/workers-sdk that referenced this pull request Feb 7, 2022
This lands support for `[wasm_modules]` as defined by cloudflare/wrangler-legacy#1677.

wasm modules can be defined in service-worker format with configuration in wrangler.toml as -

```
[wasm modules]
MYWASM = "./path/to/my-wasm.wasm"
```

The module will then be available as the global `MYWASM` inside your code. Note that this ONLY makes sense in service-worker format workers (for now).

(In the future, we MAY enable wasm module imports in service-worker format (i.e. `import MYWASM from './path/to/my-wasm.wasm'`) and global imports inside modules format workers.)
threepointone added a commit to cloudflare/workers-sdk that referenced this pull request Feb 7, 2022
This lands support for `[wasm_modules]` as defined by cloudflare/wrangler-legacy#1677.

wasm modules can be defined in service-worker format with configuration in wrangler.toml as -

```
[wasm_modules]
MYWASM = "./path/to/my-wasm.wasm"
```

The module will then be available as the global `MYWASM` inside your code. Note that this ONLY makes sense in service-worker format workers (for now).

(In the future, we MAY enable wasm module imports in service-worker format (i.e. `import MYWASM from './path/to/my-wasm.wasm'`) and global imports inside modules format workers.)
threepointone added a commit to cloudflare/workers-sdk that referenced this pull request Feb 9, 2022
This lands support for `[wasm_modules]` as defined by cloudflare/wrangler-legacy#1677.

wasm modules can be defined in service-worker format with configuration in wrangler.toml as -

```
[wasm_modules]
MYWASM = "./path/to/my-wasm.wasm"
```

The module will then be available as the global `MYWASM` inside your code. Note that this ONLY makes sense in service-worker format workers (for now).

(In the future, we MAY enable wasm module imports in service-worker format (i.e. `import MYWASM from './path/to/my-wasm.wasm'`) and global imports inside modules format workers.)
threepointone added a commit to cloudflare/workers-sdk that referenced this pull request Feb 9, 2022
This lands support for `[wasm_modules]` as defined by cloudflare/wrangler-legacy#1677.

wasm modules can be defined in service-worker format with configuration in wrangler.toml as -

```
[wasm_modules]
MYWASM = "./path/to/my-wasm.wasm"
```

The module will then be available as the global `MYWASM` inside your code. Note that this ONLY makes sense in service-worker format workers (for now).

(In the future, we MAY enable wasm module imports in service-worker format (i.e. `import MYWASM from './path/to/my-wasm.wasm'`) and global imports inside modules format workers.)
threepointone added a commit to cloudflare/workers-sdk that referenced this pull request Feb 23, 2022
This implements support for `[text_blobs]` as defined by cloudflare/wrangler-legacy#1677.

Text blobs can be defined in service-worker format with configuration in `wrangler.toml` as -

```
[text_blobs]
MYTEXT = "./path/to/my-text.file"
```

The content of the file will then be available as the global `MYTEXT` inside your code. Note that this ONLY makes sense in service-worker format workers (for now).

Workers Sites now uses `[text_blobs]` internally. Previously, we were inlining the asset manifest into the worker itself, but we now attach the asset manifest to the uploaded worker. I also added an additional example of Workers Sites with a modules format worker.
threepointone added a commit to cloudflare/workers-sdk that referenced this pull request Feb 23, 2022
This implements support for `[text_blobs]` as defined by cloudflare/wrangler-legacy#1677.

Text blobs can be defined in service-worker format with configuration in `wrangler.toml` as -

```
[text_blobs]
MYTEXT = "./path/to/my-text.file"
```

The content of the file will then be available as the global `MYTEXT` inside your code. Note that this ONLY makes sense in service-worker format workers (for now).

Workers Sites now uses `[text_blobs]` internally. Previously, we were inlining the asset manifest into the worker itself, but we now attach the asset manifest to the uploaded worker. I also added an additional example of Workers Sites with a modules format worker.
threepointone added a commit to cloudflare/workers-sdk that referenced this pull request Feb 23, 2022
This implements support for `[text_blobs]` as defined by cloudflare/wrangler-legacy#1677.

Text blobs can be defined in service-worker format with configuration in `wrangler.toml` as -

```
[text_blobs]
MYTEXT = "./path/to/my-text.file"
```

The content of the file will then be available as the global `MYTEXT` inside your code. Note that this ONLY makes sense in service-worker format workers (for now).

Workers Sites now uses `[text_blobs]` internally. Previously, we were inlining the asset manifest into the worker itself, but we now attach the asset manifest to the uploaded worker. I also added an additional example of Workers Sites with a modules format worker.
threepointone added a commit to cloudflare/workers-sdk that referenced this pull request Feb 24, 2022
This implements support for `[text_blobs]` as defined by cloudflare/wrangler-legacy#1677.

Text blobs can be defined in service-worker format with configuration in `wrangler.toml` as -

```
[text_blobs]
MYTEXT = "./path/to/my-text.file"
```

The content of the file will then be available as the global `MYTEXT` inside your code. Note that this ONLY makes sense in service-worker format workers (for now).

Workers Sites now uses `[text_blobs]` internally. Previously, we were inlining the asset manifest into the worker itself, but we now attach the asset manifest to the uploaded worker. I also added an additional example of Workers Sites with a modules format worker.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.