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.

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

pre-build before wasm-pack publish #444

Merged
merged 4 commits into from
Dec 28, 2018
Merged

Conversation

csmoe
Copy link
Member

@csmoe csmoe commented Nov 11, 2018

Closes #438

@csmoe csmoe changed the title Make sure these boxes are checked! 📦✅ pre-build before wasm-pack publish Nov 11, 2018
@csmoe csmoe changed the title pre-build before wasm-pack publish [WIP] pre-build before wasm-pack publish Nov 11, 2018
@csmoe csmoe changed the title [WIP] pre-build before wasm-pack publish pre-build before wasm-pack publish Nov 12, 2018
Copy link
Member

@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.

so- i think this is great! the one thing i'd add is that the "magic" of this feature is that it will build your project for you with the defaults. i think this is 100% in line with the design values of wasm-pack, but for the sake of the end user, i think we should emit a warning/info that says that wasm-pack has built your project with the defaults (pkg dir, and for es6 (browser target)). if you could add that i think we'll be good to merge.

if we really wanted to expand this, we could instead prompt the user if they want the project built (or if they want to cancel), and then ask them for the target they prefer. let me know what you think about that! (to skip the interactive element someone could pass wasm-pack publish --target <intended target>)

@csmoe
Copy link
Member Author

csmoe commented Dec 22, 2018

@ashleygwilliams that's great, so I'll expand the features soon as you wrote.

@csmoe
Copy link
Member Author

csmoe commented Dec 26, 2018

@ashleygwilliams pre-build flow:

build? => out_dir_name[default(pkg)]? => target[default(browser)]? => DONE

peek 2018-12-26 21-55

@csmoe csmoe force-pushed the prebuild branch 3 times, most recently from f144939 to 79f6776 Compare December 26, 2018 14:35
Copy link
Member

@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.

@csmoe this is looking AWESOME! thanks so much- do you think we could change the prompt from npm package to just package? it may also be nice to list the defaults/options for out dir and target options when prompting (e.g. when you run npm init it will show the default and you can just choose it or override) - we could also add that in a separate PR :)

so yeah, to merge- let's just switch npm package to just package- and if you'd like to, add the defaults/options

@ashleygwilliams ashleygwilliams modified the milestones: 0.7.0, 0.6.0 Dec 27, 2018
@csmoe
Copy link
Member Author

csmoe commented Dec 28, 2018

@ashleygwilliams

it may also be nice to list the defaults/options for out dir and target options when prompting

it already does, default options indicated within [...] as the demo shows(I just Enter to choose the default). but for clarifying, I'll add "default" keyword to the prompt.

@csmoe
Copy link
Member Author

csmoe commented Dec 28, 2018

peek 2018-12-28 10-54
done

Copy link
Member

@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.

this is fantastic! thank you so much!!

@ashleygwilliams ashleygwilliams merged commit 011cca9 into rustwasm:master Dec 28, 2018
@csmoe
Copy link
Member Author

csmoe commented Dec 28, 2018

@ashleygwilliams btw, may I still have your invitation of rustwasm-org as we talked in twitter several months ago?

@ashleygwilliams
Copy link
Member

oh yes! i'll take care of that today :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants