Implements RFC 8, fixing #606 #986
Conversation
|
I've pulled this branch and tested it. It looks correct. @jpgneves could you pull in the recent changes from master? |
|
@simlay Not wanting to put undue pressure, is there any chance this can be looked at? Thanks! |
| @@ -37,6 +37,7 @@ fn find_manifest_from_cwd() -> Result<PathBuf, failure::Error> { | |||
|
|
|||
| /// Construct our `pkg` directory in the crate. | |||
| pub fn create_pkg_dir(out_dir: &Path) -> Result<(), failure::Error> { | |||
| let _ = fs::remove_dir_all(&out_dir); // Clean up any existing directory and ignore errors | |||
There was a problem hiding this comment.
Hmm. It's unclear if we need to remove the entire out directory. It may not be a big deal but say someone has some npm link going on pointed at the pkg directory, this may break it on every rebuild. I'm not sure if npm link does a symlink or a hard link (or is that not a thing with directories) which I think would be the difference.
There was a problem hiding this comment.
Yeah, I can see your point. npm link documentation claims they are symlinks.
That said, I don't feel strongly about this. I personally tend to want my tests to run on a as clean as possible environment to avoid spurious failures due to other tests leaving cruft behind, but I'm happy to remove this cleanup if that makes sense to you.
There was a problem hiding this comment.
We're seeing errors that I think are in relation to this as reported at #1099 (comment)
There was a problem hiding this comment.
I've raised a PR to revert this change: #1110
|
@drager bump. |
|
Is there a timeline for when this RFC fix is going to be released? NPM dependencies still seem to be broken in |
Make sure these boxes are checked! 📦✅
rustfmtinstalledcargo fmton the code base before submitting✨✨ 😄 Thanks so much for contributing to wasm-pack! 😄 ✨✨
Fixes #606, test included.