-
Notifications
You must be signed in to change notification settings - Fork 337
Conversation
@@ -57,7 +57,7 @@ pub fn build( | |||
} | |||
TargetType::Webpack => { | |||
log::info!("Webpack project detected. Publishing..."); | |||
// FIXME(sven): shouldn't new | |||
// TODO: (sven) shouldn't new |
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.
@xtuc could you explain this 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.
i think this should actually be above ln 62, where we call wranglerjs::Bundle::new
but i'm not certain precisely why.
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.
My idea was that each backends (rust, webpack, js) returns a Bundle/WorkerBundle/Worker that contains all the bindinds. The creation of the form (code here), would consume it and build a form regardless of the backend used.
If that was implemented building a new bundle wouln't make sense. However, wranglerjs's bundle doesn't contains any state (only constants) so it's fine.
Feel free to remove the comment tho. We could also inline the constants for now.
@@ -58,6 +58,7 @@ pub fn build( | |||
TargetType::Webpack => { | |||
log::info!("Webpack project detected. Publishing..."); | |||
// TODO: (sven) shouldn't new | |||
// see https://github.com/cloudflare/wrangler/pull/846#discussion_r342845421 |
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.
I think a dedicated issue would be safer and easier to discuss in
Co-Authored-By: Ashley Michal <[email protected]>
Co-Authored-By: Ashley Michal <[email protected]>
23e2000
to
49dc0fe
Compare
This PR standardizes some comments and fixes up some typos