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

feat(rfcs): add expressing npm deps rfc #4

Closed
wants to merge 3 commits into from

Conversation

ashleygwilliams
Copy link
Member

No description provided.

@fitzgen
Copy link
Member

fitzgen commented Sep 6, 2018

Rendered

@fbstj
Copy link

fbstj commented Sep 6, 2018

any thought to having a tool to generate the package.json from the cargo.toml list?

@rvolgers
Copy link

rvolgers commented Sep 7, 2018

Worth noting that the solution for depending on external native libraries is "use a build script, possibly using pkg-config". An equivalent approach for npm would perhaps be something like a build script that uses a crate that can parse a package.json and package-lock.json and builds the proper bindings.

Obviously there are a lot of differences between dynamic linking to OS-provided versions of native libraries and static linking and bundling with npm-versioned JS libraries. But since the goal of the RFC is explicitly to look beyond just JavaScript it seems worth looking at other current cases of foreign external dependencies.

@ashleygwilliams
Copy link
Member Author

@fbstj the tool that generates a package.json from Cargo.toml would be wasm-pack. However, due to how npm packages' and rust crates' semver works, I don't think listing npm packages in a Cargo.toml is a good idea.

@ashleygwilliams
Copy link
Member Author

@rvolgers can you say more about what you mean by "But since the goal of the RFC is explicitly to look beyond just JavaScript"? The goal of this RFC is just to deal with how to express npm dependencies, containing JS- so I'm slightly confused that you say it intends to look beyond?

@rvolgers
Copy link

rvolgers commented Sep 7, 2018

@ashleygwilliams I was referring to this: https://github.com/rustwasm/rfcs/pull/4/files#diff-4a2bd8539652a54c148e82b27042e170R15 It seems like I may have misunderstood though?

Anyway, looking a bit closer into things I noticed two things that seem to make my comment invalid:

  • It seems like there is no need to actually do anything with the dependencies, since all the needed information is in the extern blocks. It's purely a concern for the bundler / runtime. I guess that's also why the RFC questions whether it needs to index the dependencies?
  • Since the npm dependency system is (a lot) more standardized than native dependencies there is no need for a high level of programmability like in a build script, and it's possible to rely fully on a one-size-fits-all tool like wasm-pack.

Sorry for wasting your time, I was just grouchy about the state of native dependency resolution and I guess I took that comment in the RFC as a reason to think about whether there was any overlap with npm dependency resolution. Turns out no, there probably isn't :)

Copy link
Member

@fitzgen fitzgen left a comment

Choose a reason for hiding this comment

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

Thanks for writing this up @ashleygwilliams!

This answers the question of what format/manifest we will use very well, and I am inclined to agree with the assessment that package.json is what we want, and with the motivation for that decision given in this RFC.

However, I am left wondering how our tooling (namely wasm-pack) will interact with this decision. Right now, we automatically generate package.json in wasm-pack. Will that continue to happen? If so, are we expected to modify this mechanically generated package.json by hand to add npm dependencies to our wasm packages? Do we author a package.json template that is merged with what wasm-pack generates? Does wasm-pack copy an [npm] section out of Cargo.toml and put the relevant bits into its generated package.json? Does wasm-pack stop generating package.json for us?

I think that the question of how wasm-pack will work with/generate this package.json is worth answering in this RFC.

Thanks again for spearheading this RFC!

- Development on Rust-generated WebAssembly projects should allow developers to use the development environment they are most comfortable with. Developers writing Rust should get to use Rust, and developers using JavaScript should get to use a JS based runtime environment (Node.js, Chakra, etc).
- JavaScript tooling and workflows should be usable with Rust-generated WebAssembly projects. For example, bundlers like WebPack and Parcel, or dependency management tools such as `npm audit` and GreenKeeper.
- When possible, decisions should be made that allow the solution to be available to developers of not just Rust, but also C, and C++.
- Decisions should be focused on creating workflows that allow developers an easy learning curve and productive development experience.
Copy link
Member

Choose a reason for hiding this comment

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

Thank you for writing down these values! They've been vaguely floating around for a while, but it is great to see them crystallized so clearly in one place.

text/003-expressing-npm-deps.md Show resolved Hide resolved
text/003-expressing-npm-deps.md Outdated Show resolved Hide resolved

Create a file called `package.json` in the root of your Rust library. Fill out dependencies as per specification: https://docs.npmjs.com/files/package.json#dependencies. You can use `npm install` to add dependencies: Although npm will warn that your `package.json` is missing metadata, it will add the dependency entry.

Note: All meta-data in this file will be ignored during `wasm-pack build` in favor of metadata in the `Cargo.toml`. The potential duplication of metadata is a downside to this solution.
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense for wasm-pack build (and wasm-pack test) to run npm install if necessary? That might be required for wasm-pack test tests to pass, for example.

Copy link

Choose a reason for hiding this comment

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

I think that depends on whether wasm-pack build will run a bundler or not (such as Webpack).

Copy link
Member Author

Choose a reason for hiding this comment

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

i'm not sure if it's related to calling a bundler, which i don't think wasm-pack will ever do. can you say more on that @Pauan ?

npm dependencies are not required to be installed in order for the package to be published, though they will be necessary to run tests. i would be inclined to have test run it since it's required, but am not convinced it's necessary for build (it's a long step and should probably only be run if necessary). am open to hearing more examples of the workflows people anticipate/already use, though.

Copy link

Choose a reason for hiding this comment

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

@ashleygwilliams My thinking is basically this: if wasm-pack doesn't run a bundler (such as Webpack), then the code will not work as-is, so it will require the user to run a bundle step themself.

And since wasm-pack isn't running a bundler, having wasm-pack build run npm install is unusual, because npm install doesn't do anything useful (since the dependencies aren't needed at this point).

So as a user it makes me wonder, "why is wasm-pack running npm install? and why isn't it running a bundler?"

I'm neutral on whether wasm-pack test should run npm install or not, though it's a little weird that it's inconsistent with wasm-build

Copy link
Member

Choose a reason for hiding this comment

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

I think running npm install for wasm-pack test but not for wasm-pack build makes sense. If we find that users are confused and this is somehow breaking their workflow, then we can revisit the decision, I think.

text/003-expressing-npm-deps.md Outdated Show resolved Hide resolved
@Pauan
Copy link

Pauan commented Sep 7, 2018

@ashleygwilliams However, due to how npm packages' and rust crates' semver works, I don't think listing npm packages in a Cargo.toml is a good idea.

I think it's worth pointing out that the Cargo.toml solution doesn't need to follow npm's semver: it can use Rust's semver instead.

And then wasm-pack would translate the Rust semver into npm semver when generating the package.json. So you'd use something like this:

[npm]
foo = "1.0.0"

And then wasm-pack would translate that into this package.json:

{
    "dependencies": {
        "foo": "^1.0.0"
    }
}


Create a file called `package.json` in the root of your Rust library. Fill out dependencies as per specification: https://docs.npmjs.com/files/package.json#dependencies. You can use `npm install` to add dependencies: Although npm will warn that your `package.json` is missing metadata, it will add the dependency entry.

Note: All meta-data in this file will be ignored during `wasm-pack build` in favor of metadata in the `Cargo.toml`. The potential duplication of metadata is a downside to this solution.
Copy link

@Pauan Pauan Sep 7, 2018

Choose a reason for hiding this comment

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

I think we should discuss having wasm-pack stop copying the metadata from Cargo.toml, instead requiring all of the metadata to be in package.json. In other words, wasm-pack would simply copy the package.json as-is.

I think this is a good idea because it means that the package.json is complete, and there are no surprises or weird behavior (everything is copied as-is), and it allows the npm package to have different metadata than the Rust package.

I can easily imagine Rust packages that might have one name on crates.io and a different name on npm, or a different description on crates.io and npm, or a different README on crates.io and npm, etc.

At the very least, if the metadata is included in the package.json then it should overwrite the metadata from Cargo.toml (with Cargo.toml being used as the default if the metadata doesn't exist in package.json)

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a very interesting point. I think I am inclined to agree! I will update the RFC. Curious if other folks have thoughts? @fitzgen @alexcrichton ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the idea of package.json overwriting Cargo.toml makes sense, but I don't think we should say that it should primarily live in package.json. This sort of metadata I believe is inherently duplicated (both NPM and Cargo want to know about a project home page and/or repository link) and as a result doesn't have a great "single source of truth". Given that this is largely Rust-centric tooling I think it makes sense to idiomatically put it in Cargo.toml. Having overrides in package.json sounds like a great idea though!

Additionally, package.json in the root crate is sort of "always inherently incomplete" because it doesn't list the NPM dependencies coming through all the various Rust dependencies. In that sense we're almost always going to need to modify package.json as-written.

@ashleygwilliams
Copy link
Member Author

Ok! So this should be updated to reflect folks comments. Largest change is that metadata in package.json will override any duplicated metadata in Cargo.toml as per @Pauan 's suggestion. Other than that, fixed the typos, added some examples.

Last thing to add is how wasm-pack will handle it (should mostly just be copying over package.json). One thing i was thinking about- @alexcrichton if we still have wasm-bindgen make the custom section that lists the npm modules used inline, wasm-pack could compare the package.json deps to that list and alert folks if they are missing a listing... thoughts?

@alexcrichton
Copy link
Contributor

@ashleygwilliams a good point! I think that's along the lines of @fitzgen's questions as well as to how we'll be transmitting this information from dependencies from wasm-bindgen to wasm-pack. I think though, yes, we'll have a custom section reserved for reading by wasm-pack. That custom section will at minimum list paths to package.json (so wasm-pack can slurp them up and add them in) and then it could also for sure list all the #[wasm_bindgen(module = "...")] declarations so wasm-pack could do some basic validation.

I'd be slightly wary about being too aggressive on warnings and such in the sense that you can't really do anything about your upstream crates.io dependencies, but warning about the local crate should work great.


### `Cargo.toml`

*Ultimately this is not a good choice because it lacks interoperability with existing JavaScript tooling, but could be considered if we anticipate that we can get tooling to use this format.*
Copy link

Choose a reason for hiding this comment

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

This doesn't actually require new tooling support, since we can use the package.metadata field in Cargo.toml:

[package.metadata.npm]
moment = "~2.22" 
#sugar for moment = { version = "~2.22", type = "prod" }
mocha = { version = "mochajs/mocha#4727d357ea", type = "dev" }
chai = { version = "^4", type = "dev" }
optional = { version = "6.6.6", type = "optional" }
git = { version = "http://asdf.com/asdf.tar.gz" }

Copy link
Member Author

Choose a reason for hiding this comment

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

i genuinely think trying to express npm deps in Cargo.toml is not the right idea- it's complicated on several levels that just using a package.json doesn't cause. is there a reason you continue to return to it?

Copy link

Choose a reason for hiding this comment

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

@ashleygwilliams What complications are you referring to? This RFC does not explain them, and it has not really been explained elsewhere either.

There are some benefits to the Cargo.toml approach:

  1. It doesn't require Rust users to learn a new format.

  2. All of the metadata for the package is in one location.

  3. The semver versions can be the same as Rust, further reducing confusion.

  4. It makes it clearer that the npm dependencies for the Rust packages will be merged (unlike package.json which implies that it is the final source of truth).

  5. It makes the workflow very clear: wasm-pack generates a package.json from Cargo.toml.

    As opposed to the package.json idea: wasm-pack merges incomplete package.jsons together and also adds in some metadata from Cargo.toml. So it's a lot more complicated from the user's perspective.

I believe that RFCs should fairly list the benefits and drawbacks (in thorough detail) of the different approaches, so that way the best decision can be made. That is what I have tried to do with my own RFC.

I don't have a particular preference (I was the one who originally suggested package.json!), I just want the best decision to be made. And I think the RFC is missing important details about the various approaches, and their respective trade-offs.

Choose a reason for hiding this comment

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

There's some community experience you might want to consider. Before npm has become the leader in Node and later all web package management, there was Bower with its own registry (like crates) and its own metadata file (bower.json, like Cargo.toml). Package maintainers who wanted to publish into both registries had to maintain two metadata files. The packages could have different names and different sets of dependencies because not everybody published to both registries, and there was (and is) no centralized registrar for package names like there are for DNS domain names. It was a mess to maintain, so Bower including the registry was quickly deprecated. Do you want to repeat the history in the Rust community? There are more and more tools emerge on top of package.json that aren't directly linked to Node packages, like dependency tree analysis on GitHub, or dependency security audit. Npm and Node as organizations for sure can do better in standardizing package.json and ways to extend it beyond Node, but it would be improvement on top of a de-facto standard in a large community of Node/web, rather than something absolutely new, quite opinionated and less portable (toml vs json) in a smaller community of Rust.

Copy link

@Pauan Pauan Sep 13, 2018

Choose a reason for hiding this comment

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

@sompylasar To be clear, the situation with Rust is not the same as npm vs bower (which I am familiar with).

In particular, Cargo will be used for Rust code (which has dependencies on other Rust packages). Then the Rust code will be compiled down to WebAssembly, and the WebAssembly will be published to npm.

So in this case we do need both cargo and npm, I don't see any way to unify them into a single packaging system.

Of course if you have any great ideas, we'd be glad to hear them, maybe we missed something!

P.S. Also, cargo is not some "absolutely new" system, it is the standard for Rust, and has been for years. There are many thousands of Rust packages already published to crates.io, and it's extremely unlikely that they will all switch from cargo to npm (even if it were technically possible to do so).

Switching to npm would actually create the exact same problem as bower: package fragmentation and duplication. It causes less fragmentation if the Rust code is published to Cargo and the WebAssembly + JS code is published to npm.

So it's not like comparing bower to npm; it's like comparing Java's Maven to npm, or Go's packaging system to npm, or C#'s nuget to npm, etc.

I've contributed to the Fable project, and they tried to consistently use npm for everything, but they ran into various problems. The unfortunate fact is that npm was designed for JavaScript, and it just doesn't work as well for non-JavaScript code.

P.P.S. What we are discussing here is not "Cargo.toml vs package.json", instead what we are discussing is "Cargo.toml by itself (and wasm-pack then creates a package.json file)" vs "Cargo.toml and package.json".

There is no scenario where we do not have a Cargo.toml file. So the Cargo.toml approach actually means one metadata file, whereas the package.json approach means two metadata files.

And no matter what is decided, the end result is that wasm-pack always generates a package.json file which is published to npm. So the npm experience will always be identical, we are only discussing the Rust user's experience.

Copy link
Member

Choose a reason for hiding this comment

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

Loving the amount of non-aggressive opinion sharing going on in this sub-thread :)

However, I feel it is getting steered a bit off course and onto tangents, and I'd like to re-focus the discussion a bit.

Let's revisit guiding values:

Development on Rust-generated WebAssembly projects should allow developers to use the development environment they are most comfortable with. Developers writing Rust should get to use Rust, and developers using JavaScript should get to use a JS based runtime environment (Node.js, Chakra, etc).

This text is taken from this very RFC, but it might as well be inside some manifesto for our working group. We want to reach out to users and work with them, however they are already working. We do not want to force them to change their workflow just to do Rust and WebAssembly hacking.

It follows that

  • Compiling a Rust crate will continue to use cargo and Cargo.toml. Trying to change that is completely out of scope, even if the Rust crate is being compiled to wasm.

  • NPM dependencies will use package.json because that's how NPM dependencies are already described in NPM-land.

These decisions are pretty much forced by the guiding values.

What is nice is that Cargo.toml dependencies are for compiling Rust to wasm, and package.json dependencies are for running compiled wasm. Note that these are two completely distinct phases, that have completely distinct sets of dependencies. We aren't in a "now we're duplicating dependencies in two places" situation. Additionally, often it isn't even the same developer(s) driving each phase: once Rust-generated wasm is published in an NPM package, then any other NPM packages that depend on it only care about the package.json dependencies. The first Rust->wasm phase might as well not exist or never have happened. And similarly, compiling Rust to wasm doesn't need to know anything about the package.json or NPM.

We are not creating an unholy amalgamation of multiple dependency configuration files and systems here. Everything is cleanly separated.

Choose a reason for hiding this comment

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

What is nice is that Cargo.toml dependencies are for compiling Rust to wasm, and package.json dependencies are for running compiled wasm.

Note that these are two completely distinct phases, that have completely distinct sets of dependencies. We aren't in a "now we're duplicating dependencies in two places" situation. Additionally, often it isn't even the same developer(s) driving each phase: once Rust-generated wasm is published in an NPM package, then any other NPM packages that depend on it only care about the package.json dependencies. The first Rust->wasm phase might as well not exist or never have happened. And similarly, compiling Rust to wasm doesn't need to know anything about the package.json or NPM.

Sounds good, but then does it mean there should be no initial package.json and everything to generate an NPM package should be in Rust ecosystem's Cargo.toml or in a separate rust-wasm-pack.toml or .json that describes this rust-to-node-wasm transformation? Let's make package.json a compilation target, and not use it as an input that, being in a repo, would confuse readers that this is a Node package while it's a Rust package that can also be built with Node and NPM as the target platform.

Copy link

Choose a reason for hiding this comment

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

I wonder how old is Rust community and how many packages are we talking about here. If there's a standard de-facto, maintainers move naturally.

Crates.io has existed for about 4 years, and in the link I posted earlier it says 18,467 crates with 528,272,511 total downloads.

I think you are severely underestimating both the age and size of the Rust community. Rust already has extremely well established de-facto (and de-jure!) standards, and it is growing at an incredibly fast rate. Given the size of the community, making any sort of change is not trivial.

Many modern tools in the Node/web world support more than one format for their metadata (e.g YAML or JavaScript for dynamic configs), but primary is JSON as it has first-class support in a lot of existing tools and protocols. TOML to me looks like a less widespread format.

As I said, it should be much easier for tools to add support for Cargo.toml (which is an established standard), rather than trying to invent a completely new standard.

You are right that JSON is probably the most well supported data format in the world, but TOML also has widespread support. And there are plenty of projects already using it. In addition, it is a very stable standard with a spec. Personally, I would say TOML is about as well supported as YAML.

Besides, parsing the data format is usually the easy part, the harder part is actually adding in code that does the dependency analysis (and this code would be the same regardless of whether it's using a new package.json metadata or Cargo.toml).

I perceive advantage in unification, update, and reuse of existing software instead of adding new software and integrating it with existing.

There is no reuse, because the package.json metadata you are proposing is a new standard, which requires new code to be written, and changes to existing packages and tools.

So the amount of work is at least as much as adding in support for Cargo.toml (and it's actually probably much more work to create a new package.json standard).

I believe teams should unite to build the package manager (not necessarily single one, but fewer is better, like Bash is de-facto standard for a shell despite there are sh, zsh, and fish).

This is an incredibly hard thing to do. I think you are overestimating just how generic npm and package.json are. There are many important things which they don't do well, such as pre-compilation, cross-compilation, optional dependencies, conditional compilation, guaranteeing one version per package, patching/overriding dependencies, pervasive global caching, etc.

The best attempt so far to create a "one package manager to rule them all" is Nix. It works extremely well, however, it works by reusing existing package managers as much as possible (e.g. it uses npm for JS packages, Cargo for Rust packages, Cabal for Haskell, etc.) This is the only real way to unify the very diverse approaches to package management.

Another attempt at a unified package manager is 0install, but in my opinion it is significantly inferior to Nix.

Like many areas in programming, package management is something that seems easy at first, but becomes incredibly hard when you start to dig into the details. The lack of a unified package manager isn't just because of people, it's also because of technical difficulties.

The reasons for the diversity in package managers are similar to the reasons for the diversity in programming languages (many attempts at a "one programming language to rule them all" have been tried, and all have failed, and not due to lack of effort).

If I sound like a junior idealist, please tell me explicitly, as despite having a decade of experience and understanding technical downsides of excessive generalization, I still believe there's a way to unite communities of different programming languages and tools to produce a better outcome overall.

I don't mean to discourage you, but frankly, yes, you do sound inexperienced in this area.

The situation with bower vs npm is not the same as Cargo vs Rust: bower and npm had similar features, were targetting a similar community, and were targetting the same language. There was a lot of overlap between them (and thus competition), which prompted developers to publish to both registries (which is not really maintainable). In addition bower was not well maintained, so npm eventually ended up winning. But history could easily have gone the other way, it was not inevitable that npm would win.

On the other hand, Cargo is fulfilling a need which is not fulfilled by any other package manager (including npm), it targets a completely different language and a different community, and it has many important features which npm lacks. Cargo is very well maintained (and constantly improving). So there is almost no overlap at all between Cargo and npm, and thus no need to publish to both Cargo and npm (as @fitzgen said, there is a very clear natural division: Rust code goes to Cargo, WebAssembly+JS code goes to npm). Therefore, I do not predict the same bower vs npm style of competition.

Sounds good, but then does it mean there should be no initial package.json and everything to generate an NPM package should be in Rust ecosystem's Cargo.toml

Yes, that is one of the potential options, and it is what I was advocating for in this specific sub-thread. I would still like to hear from @ashleygwilliams what they think the downsides of using Cargo.toml are (and why package.json is better).

Choose a reason for hiding this comment

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

Thanks for the long and great write-up, appreciate it! I internally understand all that you're saying from an engineer's perspective, and that's why I originally mentioned my wishes of united communities and fewer tools that do similar things won't ever happen. I observe so much work, which from the features viewpoint is really close, being repeated in yet another language (a multitude of languages from the past and present, now it's JavaScript, next is Rust), and this goes over and over, and this is depressing. A good example from today: I searched a dependency tree graphing tool for ES6 JS, and found a good one written in Node; why isn't there a tool for just dependency graphing (e.g. I need one for JS components and C++ components and HTTP APIs), well, because JS community won't write a generic tool, and other community won't likely write a generic tool that also supports JS. Same with package managers. Rust community made a package manager for Rust. Node community makes one for themselves. All right, wrapping up, thanks for listening.

Copy link

@Pauan Pauan Sep 14, 2018

Choose a reason for hiding this comment

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

I observe so much work, which from the features viewpoint is really close, being repeated in yet another language (a multitude of languages from the past and present, now it's JavaScript, next is Rust), and this goes over and over, and this is depressing.

I agree, but I don't think the solution is npm / package.json, instead it's something like Nix or 0install. Nix already has support for both npm and Cargo (and you can mix and match them in the same package).

In any case, it's not the Rust Wasm working group's job to push for Rust users to move to a different package manager (we are only responsible for WebAssembly, not Cargo or the overall Rust community), so the solutions presented in this RFC are the only viable ones for us.

All right, wrapping up, thanks for listening.

Thanks for sharing! I think it's good to get multiple perspectives on this issue, including from people coming from a non-Rust background.

@fitzgen
Copy link
Member

fitzgen commented Sep 11, 2018

I'd be slightly wary about being too aggressive on warnings and such in the sense that you can't really do anything about your upstream crates.io dependencies, but warning about the local crate should work great.

+1 for warnings for crates in this workspace, and not for crates elsewhere.

@fitzgen
Copy link
Member

fitzgen commented Sep 11, 2018

Really pleased with this RFC! Just one source of confusion left for me.

Will the crate's package.json just be copied into wasm-pack's pkg directory now? It won't include any automatically generated fields, such as "repository", regardless if they are present in the crate's package.json?

I guess this suggests that there will still be some mechanical generation of a package.json going on:

Note: All meta-data in this file override any duplicate fields that may be expressed in the Cargo.toml during the wasm-pack build step.

But it still isn't totally clear to me what wasm-pack is going to do automatically vs what is going to be left untouched.

Here is what I think is going to happen:

  • Initial metadata is taken from Cargo.toml; e.g. repository, homepage, authors, etc
    • This is translated into a generated package.json
  • Then, if the crate has its own local package.json, it is merged into the generated package.json.
    • If there are any keys that exist in both the generated and local package.json, then we keep the local package.json's version.
    • We don't recursively merge nested JSON objects and keys.

Essentially equivalent to this JS snippet:

const generatedPackageJson = generatePackageJsonFromCargoToml("path/to/crate/Cargo.toml");
const localPackageJson = getLocalPackageJson("path/to/crate/package.json") || {};
const finalPackageJson = Object.assign({}, generatedPackageJson, localPackageJson);

Is that correct?

I'd like to see the algorithm that wasm-pack will use for generating and merging this data (or just copying?) in the RFC. This is what I was getting at earlier with asking how wasm-pack was going to interact with this data, sorry if I wasn't clear enough.

Thanks!

@fbstj
Copy link

fbstj commented Oct 5, 2018

@ashleygwilliams your rationale in the 2018-10-04 meeting very much answers my original comment. I had somewhat groked that from the rest of this thread and by carefully reading and thinking through the text as how the sections related to one another. I agree that having the points you made written out in a way that conveys it without having to work it all out with a thinkyface, but I also can see that these RFCs might be less absolutely-rigorous and super-easy-to-read, so I don't mind if it's left as is?

@alexcrichton
Copy link
Contributor

@ashleygwilliams wanted to ping you on this and was curious if you'd have a chance to incorporate some of @fitzgen's feedback? If not I don't mind doing so as well if you're ok with that.

@alexcrichton
Copy link
Contributor

ping @ashleygwilliams, just wanted to try to follow-up again!

@ashleygwilliams
Copy link
Member Author

hey @alexcrichton @fitzgen i'm super swamped with moving- i can potentially find some time, but if i'm being totally honest it would likely be next week or later this week. if you wanted to update the RFC some go for it!

@alexcrichton
Copy link
Contributor

I've opened a new RFC for this at #8

@alexcrichton
Copy link
Contributor

I've also proposed FCP for #8

@alexcrichton
Copy link
Contributor

Since #8 is now in FCP I'm going to go ahead and close this.

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

Successfully merging this pull request may close these issues.

8 participants