-
Notifications
You must be signed in to change notification settings - Fork 335
add scaffold_worker method to Site, use all the PathBufs #851
Conversation
🤔 |
oh wow. github markdown made that weird. updated the comment. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re-approve
|
||
site | ||
} | ||
|
||
// if the user has configured `site.entry-point`, use that | ||
// as the build directory. Otherwise use the default const | ||
// SITE_ENTRY_POINT | ||
pub fn build_dir(&self, current_dir: PathBuf) -> Result<PathBuf, std::io::Error> { | ||
pub fn entry_point(&self) -> Result<PathBuf, std::io::Error> { | ||
let current_dir = env::current_dir()?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
eventually methods like these that determine where a file should be found relative to the project root should probably take as an argument the actual project root path, rather than the current directory. for the moment, everything runs relative to root because that is where wrangler.toml is and we assume you are running wrangler adjacent to the toml.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
which makes me want to write a refactor issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^ this would also be a step towards better monorepo support!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and/or just being able to run wrangler from anywhere inside a project. i'll write the issue and merge this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
|
||
site | ||
} | ||
|
||
// if the user has configured `site.entry-point`, use that | ||
// as the build directory. Otherwise use the default const | ||
// SITE_ENTRY_POINT | ||
pub fn build_dir(&self, current_dir: PathBuf) -> Result<PathBuf, std::io::Error> { | ||
pub fn entry_point(&self) -> Result<PathBuf, std::io::Error> { | ||
let current_dir = env::current_dir()?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^ this would also be a step towards better monorepo support!
fixes #622
fixes #643 by moving logic to scaffold a site worker out of wranglerjs and onto the site struct.
also some 💅:
Option<PathBuf>
instead ofOption<String>
use
statements in build/wranglerjsgenerate
, removename
argument togenerate::command
that was purely for loggingHappy to extract any controversial refactors to a separate PR, but I think this is all pretty tame.