RFC: Enable depending on NPM packages #8

alexcrichton · 2019-02-15T16:29:38Z

This proposal is an evolution of #4 which accounts for much of the discussion and makes a concrete proposal.

fitzgen

Thanks for this RFC, Alex. It provides answers for all of the open questions I had about the last iteration.

I have a few comments below, but these are nitpicks that I think we can largely address in implementation, and which don't need to block this RFC moving forward.

fitzgen · 2019-02-15T19:02:41Z

text/008-npm-dependencies.md

+The primary goal of this RFC is to enable *tranparent* and *transitive*
+dependencies on NPM. The `#[wasm_bindgen]` macro is the only aspect of a crate's
+build which has access to all transitive dependencies, so this is what we'll be
+using to slurp up `package.json`. When `#[wasm_bindgen]` with a `module` key is


Nice insight.

fitzgen · 2019-02-15T19:05:31Z

text/008-npm-dependencies.md

+(note that the cwd is set by Cargo to be the directory with the crate's
+`Cargo.toml` that is being compiled, or the crate in which `#[wasm_bindgen]` is
+written). This `package.json`, if found, will have an absolute path to it
+encoded into the custom section that `wasm-bindgen` already emits.


This dependency on package.json will be opaque to cargo -- does it make sense to always include the package.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 a package.json is included post facto should always work.

I think modifications to package.json would work incrementally either way though, right?

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)

fitzgen · 2019-02-15T19:08:16Z

text/008-npm-dependencies.md

+[alternatives]: #rationale-and-alternatives
+
+When developing this RFC, some guiding values for its design have been
+articulated:


Thank you for summarizing all of this in one place.

alexcrichton · 2019-02-15T21:49:35Z

I'd like to propose that we enter the Final Comment Period for this RFC. The original RFC has been around for quite some time and has baked for quite awhile, and I'd like to help spur on final discussion for this one before implementing!

Disposition: merge

@rustwasm/core members to sign off:

Pauan

I think this seems very reasonable for an MVP.

text/008-npm-dependencies.md

Pauan · 2019-02-16T09:52:24Z

text/008-npm-dependencies.md

+}
+```
+
+The `package.json` file will initially be a subset of NPM's `package.json`,


Are there any benefits to this restriction? Especially since we will support other fields in the future.

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 us

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.

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?

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.

alexcrichton · 2019-02-27T18:40:13Z

ping @ashleygwilliams, have you had a chance to take a look at this and review the proposal for FCP in the past two weeks?

This commit implements [RFC 8], which enables transitive and transparent dependencies on NPM. The `module` attribute, when seen and not part of a local JS snippet, triggers detection of a `package.json` next to `Cargo.toml`. If found it will cause the `wasm-bindgen` CLI tool to load and parse the `package.json` within each crate and then create a merged `package.json` at the end. [RFC 8]: rustwasm/rfcs#8

alexcrichton · 2019-02-28T19:39:42Z

I've got a sample implementation of the currently proposed version of the RFC at rustwasm/wasm-bindgen#1305

alexcrichton · 2019-03-05T15:46:34Z

@ashleygwilliams another ping since last week to check the FCP proposal

alexcrichton · 2019-03-13T14:37:21Z

@ashleygwilliams just another ping to check out the FCP proposal, would be great if we could get this in the near future!

chinedufn · 2019-03-13T16:36:45Z

@alexcrichton not to speak for Ashley but I noticed from twitter that she's recently moved cross country and changed jobs so I would imagine that she's super busy.

Not my intention to speak for her - just letting you know in case you don't already!

(@ashleygwilliams I'm really not trying to speak for you and just trying to call out that you might be a bit busy right now so feel free to let me know if I'm totally wrong!!!)

alexcrichton · 2019-03-13T17:54:47Z

@chinedufn true! I'm trying to only ping at most once a week though because I want to make sure this doesn't fall off the radar but it also isn't too overwhelming. We currently require a full-team signoff (although that may change in #9), so we're unfortunately stuck leaving out this feature (which I personally feel is very important!) until @ashleygwilliams checks her box.

chinedufn · 2019-03-13T19:03:09Z

Sounds good - hadn't seen #9 - thanks!

alexcrichton · 2019-03-14T18:00:44Z

🔔 🔔 🔔

This RFC has entered its final comment period. In seven calendar days, assuming no substantial new arguments or ideas are raised, we will merge it.

This commit implements [RFC 8], which enables transitive and transparent dependencies on NPM. The `module` attribute, when seen and not part of a local JS snippet, triggers detection of a `package.json` next to `Cargo.toml`. If found it will cause the `wasm-bindgen` CLI tool to load and parse the `package.json` within each crate and then create a merged `package.json` at the end. [RFC 8]: rustwasm/rfcs#8

trivigy · 2019-12-11T20:45:39Z

@alexcrichton The restriction just broke the builds! As part of the workflow scripts are included in package.json. In particular, when using webpack to do extra automation for scss, css, and a bunch of other bundling as well as prettier, and storybooks, and more. To execute on all of them and more there is a need for something like this:

{
  "scripts": {
    "clean": "rm -rf ./pkg ./target ./dist ./node_modules",
    "build": "webpack",
    "serve": "webpack-dev-server",
    "storybook": "start-storybook -p 6006",
    "format": "prettier --write \"./src/**/*.{js,scss}\""
  },
  "devDependencies": {
    "@babel/core": "^7.7.4",
    "@babel/plugin-syntax-dynamic-import": "^7.7.4",
    "@storybook/addon-actions": "^5.2.8",
    "@storybook/addon-knobs": "^5.2.8",
    "@storybook/addons": "^5.2.8",
    "@storybook/html": "^5.2.8",
    "@wasm-tool/wasm-pack-plugin": "^1.0.1",
    "babel-loader": "^8.0.6",
    "babel-plugin-macros": "^2.7.1",
    "clean-webpack-plugin": "^3.0.0",
    "copy-webpack-plugin": "^5.0.4",
    "css-loader": "^3.2.0",
    "fibers": "^4.0.2",
    "html-webpack-plugin": "^3.2.0",
    "paths.macro": "^2.0.2",
    "prettier": "^1.19.1",
    "react": "^16.12.0",
    "react-dom": "^16.12.0",
    "sass": "^1.23.1",
    "sass-loader": "^8.0.0",
    "style-loader": "^1.0.0",
    "ulid": "^2.3.0",
    "webpack": "^4.29.4",
    "webpack-cli": "^3.1.1",
    "webpack-dev-server": "^3.1.0"
  }
  }
}

In addition. Please note, that the dependencies these tools require are "devDependencies" only. I wouldn't want them to be included in the "dependencies" key in the first place. Also important to note here the use of @wasm-tool/wasm-pack-plugin. This one is a must because it is the only way to integrate into the storybook custom build process. They support an additional webpack.config.js file placed inside of .storybook directory.

Not entirely sure how I could work around such limitation of "only dependencies". Would appreciate some input.

trivigy · 2019-12-11T21:49:45Z

On a separate note, this RFC is extremely nuanced because the only reason this issue surfaced in my specific use case was due to the use of #[wasm_bindgen(module = "foo")] module dependency. Without in, there is no lookup and merging of the package.json and therefore this change was never noticed until now.

alexcrichton · 2019-12-12T22:27:44Z

Sorry for the difficulties you're having @trivigy! I think that unfortunately there's not a great answer today, but brainstorming on how to solve this would be greatly appreciated!

trivigy · 2019-12-12T23:27:34Z

@alexcrichton I ended up spending the time rewriting everything in terms of inline_js. That's my work around for now. But I bet this exact issue will come up for others as well. Thanks for the hard work you put in and for responding.

DarthGandalf · 2019-12-14T18:53:54Z

This broke me too. I use npm install to install npm dependencies, followed by npm run build which uses webpack which invokes wasm-pack via @wasm-tool/wasm-pack-plugin. Can there be an option for wasm-pack to allow any keys in package.json and not require user to remove all keys from it which would break other tools which read it?

There are lots of tools which use package.json other than the dependencies key:

While some of them have alternative ways to configure them, breaking a common practice of using package.json for the sake of better integration with NPM ecosystem is self-contradicting.

Please remove this restriction and simply ignore unknown keys.

alexcrichton · 2019-12-16T15:30:20Z

@DarthGandalf at this point it's probably best to open an issue and/or PR on wasm-bindgen, and please feel free to do so!

Pauan · 2019-12-16T23:44:42Z

I've given some thought to this, and I agree that we should allow for all keys in package.json. This makes it possible to use 1 package.json file (rather than 2), and it also matches what npm itself does.

RFC: Enable depending on NPM packages

8871ee0

alexcrichton mentioned this pull request Feb 15, 2019

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

Closed

fitzgen reviewed Feb 15, 2019

View reviewed changes

Pauan approved these changes Feb 16, 2019

View reviewed changes

Pauan mentioned this pull request Feb 17, 2019

How to write a wrapper for an existing JS library? rustwasm/wasm-bindgen#1265

Closed

Tweak version req

57cc2dc

This was referenced Feb 20, 2019

Interaction between Rust wasm/foreign modules and encapsulation rustwasm/team#92

Closed

Use node_modules (Node.js) in WASM ? rustwasm/team#232

Closed

alexcrichton mentioned this pull request Feb 28, 2019

Implement transitive support for NPM dependencies rustwasm/wasm-bindgen#1305

Merged

fitzgen mentioned this pull request Mar 7, 2019

Add an amendment to the RFC process #9

Open

alexcrichton added the sprint label Mar 14, 2019

fitzgen merged commit 439fe1c into rustwasm:master Mar 21, 2019

trivigy mentioned this pull request Dec 11, 2019

Implement support for RFC 8, transitive NPM dependencies rustwasm/wasm-pack#606

Closed

bminixhofer mentioned this pull request May 4, 2020

NPM dependencies don't get copied to pkg/package.json rustwasm/wasm-pack#840

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RFC: Enable depending on NPM packages #8

RFC: Enable depending on NPM packages #8

alexcrichton commented Feb 15, 2019 •

edited

Loading

fitzgen left a comment

fitzgen Feb 15, 2019

fitzgen Feb 15, 2019

alexcrichton Feb 15, 2019

fitzgen Feb 15, 2019

alexcrichton commented Feb 15, 2019 •

edited by ashleygwilliams

Loading

Pauan left a comment

Pauan Feb 16, 2019

alexcrichton Feb 19, 2019

Pauan Feb 19, 2019 •

edited

Loading

alexcrichton Feb 19, 2019

Pauan Feb 19, 2019

alexcrichton commented Feb 27, 2019

alexcrichton commented Feb 28, 2019

alexcrichton commented Mar 5, 2019

alexcrichton commented Mar 13, 2019

chinedufn commented Mar 13, 2019

alexcrichton commented Mar 13, 2019

chinedufn commented Mar 13, 2019

alexcrichton commented Mar 14, 2019

trivigy commented Dec 11, 2019 •

edited

Loading

trivigy commented Dec 11, 2019

alexcrichton commented Dec 12, 2019

trivigy commented Dec 12, 2019 •

edited

Loading

DarthGandalf commented Dec 14, 2019

alexcrichton commented Dec 16, 2019

Pauan commented Dec 16, 2019

RFC: Enable depending on NPM packages #8

RFC: Enable depending on NPM packages #8

Conversation

alexcrichton commented Feb 15, 2019 • edited Loading

fitzgen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcrichton commented Feb 15, 2019 • edited by ashleygwilliams Loading

Pauan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Pauan Feb 19, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcrichton commented Feb 27, 2019

alexcrichton commented Feb 28, 2019

alexcrichton commented Mar 5, 2019

alexcrichton commented Mar 13, 2019

chinedufn commented Mar 13, 2019

alexcrichton commented Mar 13, 2019

chinedufn commented Mar 13, 2019

alexcrichton commented Mar 14, 2019

trivigy commented Dec 11, 2019 • edited Loading

trivigy commented Dec 11, 2019

alexcrichton commented Dec 12, 2019

trivigy commented Dec 12, 2019 • edited Loading

DarthGandalf commented Dec 14, 2019

alexcrichton commented Dec 16, 2019

Pauan commented Dec 16, 2019

alexcrichton commented Feb 15, 2019 •

edited

Loading

alexcrichton commented Feb 15, 2019 •

edited by ashleygwilliams

Loading

Pauan Feb 19, 2019 •

edited

Loading

trivigy commented Dec 11, 2019 •

edited

Loading

trivigy commented Dec 12, 2019 •

edited

Loading