Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Split init into init and build #216

Merged
merged 7 commits into from
Jul 30, 2018
Merged

Split init into init and build #216

merged 7 commits into from
Jul 30, 2018

Conversation

csmoe
Copy link
Member

@csmoe csmoe commented Jul 16, 2018 •
edited
Loading

@ashleygwilliams Sorry for missing your comment there since I was nearly done at that time.

@csmoe csmoe force-pushed the build branch 2 times, most recently from 24dac77 to 617b996 Compare July 16, 2018 23:38
@csmoe csmoe changed the title [WIP] Split init into init and build Split init into init and build Jul 16, 2018
@ashleygwilliams ashleygwilliams added needs review needs docs please add docs to this PR labels Jul 17, 2018
@ashleygwilliams
Copy link
Member

looking great so far @csmoe thanks for taking this on! this will def require some docs updates! i can get you a review by tomorrow, but in the meantime- if you want to add docs that would be great!

@ashleygwilliams ashleygwilliams added this to the 0.5.0 milestone Jul 17, 2018
@mgattozzi
Copy link
Contributor

mgattozzi commented Jul 24, 2018 •
edited
Loading

Hey @csmoe mind rebasing and fixing the conflicts? I can give you a review as soon as that's done now that I'm back :) This way we can also get #194 reviewed and in as well :D

@csmoe
Copy link
Member Author

csmoe commented Jul 24, 2018

@mgattozzi will rebase soon.

@csmoe csmoe force-pushed the build branch 3 times, most recently from 88f868a to 7ce7a2a Compare July 25, 2018 03:48
@ashleygwilliams
Copy link
Member

hey @csmoe - sorry it's taken me so long to review this. so just to be sure i understand, the current code- when you run wasm-pack build it runs wasm-pack init and then the build steps? i think our goal would be to keep those separate? e.g. if you run wasm-pack build and haven't run init it asks you to run init first? tbh i'm not entirely sure, i mostly want to make build separate so that folks can run it multiple times (e.g. debug first, and then trying out diff configs a la #153)

anyways, i think for the most part this is ready to merge (needs a rebase cuz i merged #211 but i'm happy to handle that for you) but we still have some questions about workflow/UX.

i'm meeting with @alexcrichton and @fitzgen to talk more about the user flow later today, so i can update after that meeting- but if you have thoughts, please share!

@ashleygwilliams ashleygwilliams mentioned this pull request Jul 26, 2018
Closed
@csmoe
Copy link
Member Author

csmoe commented Jul 26, 2018

@ashleygwilliams I copied the workflow from your comment there:

  • init becomes what init --mode no-build is now
  • build is everything --mode no-build skips, build should have a --no-installs flag

I was also puzzled by the repeated init-steps in build(sorry for not asking for your suggestion at that time), but I just simply left it as it was.

i think our goal would be to keep those separate? e.g. if you run wasm-pack build and haven't run init it asks you to run init first?

I love this.

@csmoe csmoe force-pushed the build branch 4 times, most recently from fcbb1f0 to b8ce2b4 Compare July 27, 2018 03:10
@ashleygwilliams ashleygwilliams mentioned this pull request Jul 27, 2018
Closed
xtuc
xtuc approved these changes Jul 28, 2018
View reviewed changes
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.

Works great for me! thanks

docs/init.md
@@ -1,9 +1,9 @@
# wasm-pack init
# wasm-pack init(Deprecated)
Copy link
Member

Choose a reason for hiding this comment

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

nit: space before (

src/command/build.rs
/// this flag will disable generating this TypeScript file.
pub disable_dts: bool,

#[structopt(long = "target", short = "t", default_value = "browser")]
Copy link
Member

Choose a reason for hiding this comment

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

btw browser is not accurate. ESM will be soon available in Nodejs, which isn't a browser.

The correct targets are:

  • CJS (commonjs)
  • ESM (ECMAScript modules)

Copy link
Member Author

Choose a reason for hiding this comment

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

ESM will be soon available in Nodejs, which isn't a browser.

so should I leave a FIXME note here or change it to ESM right now?

Copy link
Member

Choose a reason for hiding this comment

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

leave whatever the current behavior is, and let's file an issue to update.

mgattozzi
mgattozzi reviewed Jul 30, 2018
View reviewed changes
Copy link
Contributor

@mgattozzi mgattozzi left a comment

Choose a reason for hiding this comment

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

I won't block on this, but considering it's in the readme it's a bit more visual so I think we should fix this doc now. Anyways, you did a great job @csmoe :D

README.md
@@ -29,9 +29,10 @@ visiting that repo!

## 🎙️ Commands

- [`init`](docs/init.md): [**Deprecated**] Initialize an npm wasm pkg from a rustwasm crate
- [`build`](docs/build.md): Generate an npm wasm pkg from a rustwasm crate
- [`init`](docs/init.md): Generate an npm wasm pkg from a rustwasm crate
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this line wasn't removed despite having added the deprecated tag

@ashleygwilliams ashleygwilliams merged commit 883cae2 into rustwasm:master Jul 30, 2018
@ashleygwilliams ashleygwilliams mentioned this pull request Jul 30, 2018
Closed
@data-pup data-pup mentioned this pull request Aug 25, 2018
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mgattozzi mgattozzi mgattozzi left review comments

@xtuc xtuc xtuc approved these changes

@ashleygwilliams ashleygwilliams ashleygwilliams approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.5.0
Development

Successfully merging this pull request may close these issues.
4 participants
@csmoe @ashleygwilliams @mgattozzi @xtuc