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

feat: allow custom webpack configuration in wrangler.toml #253

Merged
merged 2 commits into from
Jun 21, 2019
Merged

feat: allow custom webpack configuration in wrangler.toml #253

merged 2 commits into from
Jun 21, 2019

Conversation

EverlastingBugstopper
Copy link
Contributor

@EverlastingBugstopper EverlastingBugstopper commented Jun 14, 2019

Fixes #246

This PR allows wrangler to work with projects like this one: https://github.com/jrf0110/cf-workers-boilerplate

@EverlastingBugstopper
Copy link
Contributor Author

Side note: Rust is so cool. This is my first Rust PR, I would love feedback on what could be better. I'm not sure if my patterns are the best.

Copy link
Member

@xtuc xtuc left a comment

Choose a reason for hiding this comment

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

This is looking really good!

@EverlastingBugstopper
Copy link
Contributor Author

I added another test case to make sure that a specified webpack configuration file with multiple exports will throw an error

@EverlastingBugstopper
Copy link
Contributor Author

Ran cargo fmt

@EverlastingBugstopper
Copy link
Contributor Author

Ran cargo clippy

@xtuc xtuc modified the milestone: 1.1.0 Jun 14, 2019
Copy link
Contributor

@ashleygwilliams ashleygwilliams left a comment

Choose a reason for hiding this comment

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

i think this looks great in general- a few small notes, but nothing blocking! let's get this in

webpack_config = "webpack.worker.js"
"#};

build_fails_with(fixture, "Multiple webpack configurations are not supported. You can specify a different path for your webpack configuration file in wrangler.toml with the `webpack_config` field");
Copy link
Contributor

Choose a reason for hiding this comment

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

long term... testing on the full error message might make this test a little brittle. i think it's totally fine to keep now, but if we find the error message changes slightly a lot over time... we may just want to test that the error message contains some keywords that we know won't change and are enough to identify it :)

let webpack_config_path = PathBuf::from(
&project
.webpack_config
.clone()
Copy link
Contributor

Choose a reason for hiding this comment

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

ideally we can avoid clones but sometimes it's not ideal for code readability :) that being said i think this is fine for now, we should just audit these generally

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I didn't really like it, just what the compiler recommended. I'm still a little iffy on how Rust handles borrowing + what is idiomatic.

@ashleygwilliams
Copy link
Contributor

@EverlastingBugstopper do you think you'd be able to work with @victoriabernard92 + @signalnerve to make a small template that demonstrates using a user created webpack config? i think it'd be great to release this alongside an example

@EverlastingBugstopper
Copy link
Contributor Author

@ashleygwilliams Yeah I could do that. Would that be a part of this PR or separate?

@ashleygwilliams
Copy link
Contributor

@EverlastingBugstopper let's do it in a different one! landing this now :)

@ashleygwilliams ashleygwilliams merged commit ba729f8 into cloudflare:master Jun 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow the user to specify an alternative webpack.config.js file
4 participants