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

[Wrangler Ignore option 2] exclude/include logic in wrangler.toml for static asset uploads #760

Merged
merged 39 commits into from
Oct 7, 2019

Conversation

gabbifish
Copy link
Contributor

@gabbifish gabbifish commented Oct 3, 2019

This PR is the secondary implementation/option to close #716. The first option is #720.

Wrangler will look at wrangler.toml's site section for fields to ignore:

[site]
bucket = "./public"
entry-point = "workers-site" 
include = ["included_dir"] # optional
exclude = ["excluded_dir"] # optional

The include and exclude fields will apply to the bucket directory. Its behavior above is exactly the same as https://github.com/ashleygwilliams/cargo-generate#include--exclude (this is also the standard for cargo).

Keep in mind that, consistent with other implementations of include/exclude, the include field always overrides the exclude one.

This PR also contains a bunch of unit tests to ensure that our exclude/include logic works as expected.

gabbifish and others added 30 commits September 25, 2019 21:06
…le. This approach should be readily applicable to .wignore logic down the line
* Add message for file ignore

* Only build asset manifest once (#717)

* Add AssetManifest type

* Only manifest building logic once
…le. This approach should be readily applicable to .wignore logic down the line
* Add message for file ignore

* Only build asset manifest once (#717)

* Add AssetManifest type

* Only manifest building logic once
@gabbifish gabbifish added feature Feature requests and suggestions status - needs review labels Oct 3, 2019
@gabbifish gabbifish added this to the 1.5.0 milestone Oct 3, 2019
@gabbifish gabbifish self-assigned this Oct 3, 2019
@gabbifish gabbifish added regression Something is broken, but works in previous releases sites labels Oct 3, 2019
Copy link
Contributor

@EverlastingBugstopper EverlastingBugstopper left a comment

Choose a reason for hiding this comment

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

This looks really good!

There are a couple of things I noticed while testing this:

  1. There is no output that lets you know what files it is ignoring (perhaps a --verbose would be good?)
  2. At first I tried excluding directories with relative paths (./img) but that didn't work - perhaps if something can't be parsed in the include/exclude we should bail before uploading stuff they might want to exclude
  3. If you put in a string instead of a sequence, the error message isn't the best (but you can probably still figure stuff out)
$ wrangler publish
Error: invalid type: string "./img", expected a sequence
  1. it might be useful to have some log::info or log::debug statements - especially for things like the default to ignore node_modules

src/commands/kv/bucket/mod.rs Show resolved Hide resolved
src/commands/kv/bucket/mod.rs Show resolved Hide resolved
src/commands/kv/bucket/mod.rs Outdated Show resolved Hide resolved
@gabbifish
Copy link
Contributor Author

gabbifish commented Oct 7, 2019

@EverlastingBugstopper responding to questions:

  1. There is no output about what's being ignored, given that the user explicitly already states what should be ignored in the wrangler.toml file. I can add some info text about node_modules being ignored, but it's unlikely folks will have node_modules in their bucket in the first place. Perhaps we can just inform folks about this via the docs for now.
  2. You can't really use .gitignore with the paths like ./img--it's a bit of an antipattern, you usually just provide img/. The same applies for this approach with exclude/include. This is because these types of ignore patterns can come from a .ignore file anywhere in a project, so using relative paths dependent on . is unsafe. This isn't a problem with parsing, but rather a problem with whether a user is using general ignore/exclude semantics conventionally.
  3. I can look into serde and see if we can make the error message a bit better and remind folks to use an array if they just provide a string. <- this is non-trivial, can we shelve this?
  4. I'll add info logs for everything ignored. :)

@EverlastingBugstopper
Copy link
Contributor

Makes sense - re no. 2, do you think it makes sense to print a message::warn when something that is in an include or an exclude is not recognized? is that possible? like "hey looks like you wanna ignore this but i have no idea what that even is". maybe not necessary but might be helpful. the info logs might be enough for this

Copy link
Contributor

@EverlastingBugstopper EverlastingBugstopper left a comment

Choose a reason for hiding this comment

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

lgtm

@gabbifish
Copy link
Contributor Author

gabbifish commented Oct 7, 2019

@EverlastingBugstopper I might wait until we see a ticket related to include/exclude entries starting with ./ -- I feel like this case is unlikely, but if an issue is filed, I will definitely add additional hand-holding logic!

@gabbifish gabbifish merged commit 753d8f3 into master Oct 7, 2019
@delete-merged-branch delete-merged-branch bot deleted the gabbi/wrangler-exclude-include branch October 7, 2019 22:45
@ashleygwilliams ashleygwilliams added changelog - feature and removed feature Feature requests and suggestions labels Oct 25, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
regression Something is broken, but works in previous releases sites
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RFC: .wranglerignore
3 participants