feat: implement rules config field
#530
Conversation
|
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/1912742590/npm-package-wrangler-530You can reference the automatically updated head of this PR with: npm install --save-dev https://prerelease-registry.developers.workers.dev/prs/530/npm-package-wrangler-530Or you can use npx https://prerelease-registry.developers.workers.dev/runs/1912742590/npm-package-wrangler-530 dev path/to/script.js |
|
Why didn't the changeset bot drop a comment here? 🤔 I'll investigate later, unless someone else has an idea |
threepointone
force-pushed
the
build-upload-rules
branch
from
22aa1a4
to
5ef8cf2
Compare
🦋 Changeset detectedLatest commit: 9cfaec8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
threepointone
force-pushed
the
build-upload-rules
branch
from
5ef8cf2
to
0d3c6c6
Compare
| { type: "Text", globs: ["**/*.file"], fallthrough: true }, | ||
| { type: "Text", globs: ["**/*.other"], fallthrough: true }, |
There was a problem hiding this comment.
Not related to this PR, but I don't understand how this is different to:
{ type: "Text", globs: ["**/*.file", **/*.other"] },
and if so, what the point of fallthrough is?
🤔
There was a problem hiding this comment.
If it is not useful (and just adds unnecessary complexity), how about we just don't support fallthrough as a breaking change?
There was a problem hiding this comment.
Hmm, OK. So it seems that this is used to allow custom rules to fallback to the DEFAULT_MODULE_RULES...
There was a problem hiding this comment.
or any rule that follows. It's a weird one, but I have to preserve the behaviour.
There was a problem hiding this comment.
Right, but apart from the default ones, the developer of the worker has full control over this list, so they could just put all the globs that they want to match into a single rule for a particular type.
There was a problem hiding this comment.
Correct, but that would be a breaking change. I didn't want to introduce that in this PR. Happy to do so in a subsequent one.
| let err: Error | undefined; | ||
| try { | ||
| await runWrangler("publish index.js"); | ||
| } catch (e) { | ||
| err = e as Error; | ||
| } | ||
| expect(err?.message).toMatch( | ||
| `The file ./other.other matched a module rule in your configuration ({"type":"Text","globs":["**/*.other"]}), but was ignored because a previous rule with the same type had \`fallthrough = false\`.` | ||
| ); |
There was a problem hiding this comment.
Why not use:
await expect(runWrangler("publish index.js")).rejects.toThrowError(...);(or similar but with an inline snapshot)?
There was a problem hiding this comment.
because the full error contains a file path specific to the machine (so in my case, it contained my home dir etc) and I needed to match only with part of the error.
There was a problem hiding this comment.
Hmm. So I tried rejects.toThrowError() with the same string you provide here and it worked; so I am not sure what the problem is.
To be clear I tried:
await expect(runWrangler("publish index.js")).rejects.toThrowError(
`The file ./other.other matched a module rule in your configuration ({"type":"Text","globs":["**/*.other"]}), but was ignored because a previous rule with the same type had \`fallthrough = false\`.`
);There was a problem hiding this comment.
oh damn you're right, I was trying toMatchInlineSnapshot
| fs.writeFileSync("./other.other", "SOME OTHER TEXT CONTENT"); | ||
|
|
||
| // We throw an error when we come across a file that matched a rule | ||
| // but was skipped because of faullthrough = false |
There was a problem hiding this comment.
| // but was skipped because of faullthrough = false | |
| // but was skipped because of fallthrough = false |
| fs.writeFileSync("./other.other", "SOME OTHER TEXT CONTENT"); | ||
|
|
||
| // We throw an error when we come across a file that matched a rule | ||
| // but was skipped because of faullthrough = false |
There was a problem hiding this comment.
| // but was skipped because of faullthrough = false | |
| // but was skipped because of fallthrough = false |
| // eslint-disable-next-line @typescript-eslint/no-non-null-assertion | ||
| .concat(DEFAULT_MODULE_RULES!) |
There was a problem hiding this comment.
This is a good reason for the Config not to have unnecessary optional properties.
| } else { | ||
| } |
There was a problem hiding this comment.
| } else { | |
| } | |
| } |
| index++; | ||
| } | ||
|
|
||
| // eslint-disable-next-line @typescript-eslint/no-non-null-assertion |
| .update(fileContent) | ||
| .digest("hex"); | ||
| const fileName = `${fileHash}-${path.basename(args.path)}`; | ||
| rules?.forEach((rule) => { |
| // with an underscore. | ||
| // TODO: what of "Data"? | ||
| contents: `export default ${args.path.replace( | ||
| /[^a-zA-Z0-9_$]/g, |
There was a problem hiding this comment.
Do we need to ensure that the first character is not a number?
There was a problem hiding this comment.
It's always a relative path, so I didn't bother (unless I'm wrong?)
There was a problem hiding this comment.
Do you mean that these paths always start with ./, which will be converted to __?
| args.path | ||
| } matched a module rule in your configuration (${JSON.stringify( | ||
| rule | ||
| )}), but was ignored because a previous rule with the same type had \`fallthrough = false\`.` |
There was a problem hiding this comment.
Strictly, I think it is ignored because a previous rule did not have fallthrough = true, right?
There was a problem hiding this comment.
Good catch, yes.
| console.warn( | ||
| `Deprecation notice: The \`build.upload.rules\` config field is no longer used, the rules should be specified via the \`rules\` config field. Delete the \`build.upload\` field from the configuration file, and add this: | ||
|
|
||
| ${TOML.stringify({ rules: config.build.upload.rules })}` |
threepointone
force-pushed
the
build-upload-rules
branch
from
0d3c6c6
to
e9cbc40
Compare
This implements the top level `rules` configuration field. It lets you specify transport rules for non-js modules. For example, you can specify `*.md` files to be included as a text file with -
```
[[rules]]
{type = "Text", globs = ["**/*.md"]}
```
We also include a default ruleset -
```
{ type: "Text", globs: ["**/*.txt", "**/*.html"] },
{ type: "Data", globs: ["**/*.bin"] },
{ type: "CompiledWasm", globs: ["**/*.wasm"] },
```
More info at https://developers.cloudflare.com/workers/cli-wrangler/configuration/#build.
Known issues -
- non-wasm module types do not work in `--local` mode
- `Data` type does not work in service worker format, in either mode
threepointone
force-pushed
the
build-upload-rules
branch
from
e9cbc40
to
9cfaec8
Compare
This implements the top level
rulesconfiguration field. It lets you specify transport rules for non-js modules. For example, you can specify*.mdfiles to be included as a text file with -We also include a default ruleset -
More info at https://developers.cloudflare.com/workers/cli-wrangler/configuration/#build.
Known issues -
--localmodeDatatype does not work in service worker format, in either mode