Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
RFC: Enable depending on NPM packages #8
RFC: Enable depending on NPM packages #8
Changes from 1 commit
8871ee0
57cc2dc
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Are there any benefits to this restriction? Especially since we will support other fields in the future.
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'm mostly thinking that we want to start out conservative. In the end wasm-bindgen will be producing a "merged" package.json, but that means that we in theory need a merging function for all fields of package.json. For now it's easy enough for us to define how to merge
dependencies
, but beyond that I figure we'd want to make explicit decisions about how to merge other fields rather than having something accidentally ad-hoc-ly define how to do it for usThere 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.
Could we somehow error for transitive crates but not the current crate?
Since the current crate might be published to npm, so it might want things like
name
,version
, etc.But transitive crates aren't intended for publishing to npm, so it's okay to restrict them to
dependencies
only.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.
It's possible I think! My hope though is that only allowing
dependencies
is a useful-enough MVP to build on a feature like this in the future. Do you think, though, that we need to allow other fields (in some possible form) in the initial pass to be useful enough though?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 suspect that at least
version
will be needed, since the npm version can be different from the Rust version (e.g. breaking changes in.rs
but not in compiled.wasm
).Maybe not needed for the MVP though, so I don't think it should block anything. I think this RFC is fine as-is.
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.
Nice insight.
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 dependency on package.json will be opaque to
cargo
-- does it make sense to always include thepackage.json
path (even when it doesn't exist) in the custom section and have the CLI tool handle the missing case? This way incremental builds where apackage.json
is included post facto should always work.I think modifications to package.json would work incrementally either way though, right?
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.
Actually now that I think about it I think we can get rustc/cargo to learn about this dependency via
include_str!
. We could probably emit a dummy constant that doesn't end up getting used to force it to get included. That way the proc macro could actually work with the contents rather than just the path (and could put the entire contents in the custom section)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.
Thank you for summarizing all of this in one place.