Skip to content

NPM dependencies don't get copied to pkg/package.json #840

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.

Sign up for GitHub

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

Jump to bottom
Open
bminixhofer opened this issue May 4, 2020 · 1 comment
Open

NPM dependencies don't get copied to pkg/package.json #840

bminixhofer opened this issue May 4, 2020 · 1 comment

Comments

@bminixhofer
Copy link

bminixhofer commented May 4, 2020 •
edited
Loading

🐛 Bug description

Dependencies declared in a package.json next to Cargo.toml do not get copied over to pkg/package.json. This should be supported according to rustwasm/rfcs#8.

Content of handwritten package.json:

{
    "dependencies": {
        "@tensorflow/tfjs": "^1.7.4"
    }
}

Content of generated pkg/package.json:

{
  "name": "npm-dep-test",
  "collaborators": [
    "Benjamin Minixhofer <[email protected]>"
  ],
  "version": "0.1.0",
  "files": [
    "npm_dep_test_bg.wasm",
    "npm_dep_test.js",
    "npm_dep_test.d.ts"
  ],
  "module": "npm_dep_test.js",
  "types": "npm_dep_test.d.ts",
  "sideEffects": false
}

🤔 Expected Behavior

I would have expected pkg/package.json to have a "dependencies" key which lists "@tensorflow/tfjs" as dependency.

👟 Steps to reproduce

To reproduce:

  1. Generate a new project: cargo generate --git https://github.com/rustwasm/wasm-pack-template
  2. Create a file package.json next to Cargo.toml at the project root, which has the content:
{
    "dependencies": {
        "@tensorflow/tfjs": "^1.7.4"
    }
}
  1. (probably optional) add some code to src/lib.rs which uses the dependency to make sure it doesn't get optimized out, e. g.
#[wasm_bindgen(module = "@tensorflow/tfjs")]
extern {
    fn sequential() -> JsValue;
}

#[wasm_bindgen]
pub fn greet() {
    alert(&format!("{:#?}", sequential()));
}
  1. Run wasm-pack build and look at the content of pkg/package.json.

Alternatively, run git clone https://github.com/bminixhofer/npm-dep-test to skip steps 1-3.

🌍 Your environment

Include the relevant details of your environment.
wasm-pack version: wasm-pack 0.9.1
rustc version: rustc 1.43.0 (4fb7144ed 2020-04-20)

Related
abernix added a commit to apollographql/federation that referenced this issue Sep 23, 2020
@abernix
Setup CircleCI/Lerna/npm to compile and publish `@apollo/query-planne…
c10e367 
…r-wasm`

This allows us to compile and depend upon the local version of the WASM
query planner package and get the tarballs that are packed with the right
version.  Additionally, this allows Lerna to handle the version bumping and
publishing for `@apollo/query-planner-wasm`, since it is uniquely positioned
to do that best in a monorepo orchestration that involves multiple npm
packages with relative `file:` dependencies on each other and coupled with
our existing needs for Lerna and publishing workflows we have built.

Technically speaking, this relieves the `Cargo.toml` from its responsibility
of managing the `version` and shifts that responsibility to `lerna` and
other npm-specific tooling we have connected to that.  In fact, this change
completely eliminates the need for the `wasm-pack` generated `package.json`
that is derived from the Cargo.toml (and rendered into the "out-dir",
previously `pkg/`).

While I would believe that there are some advanced `wasm-pack` cases where
this above change might be undesirable, for now, the constraint incurred by
that transposition is unblocking in other ways.  For example, rather than
necessitating parallel metadata support in the `Cargo.toml` for every
`package.json` property (think about npm specific properties like "labels",
"bugs", "private"), we can simply change our `package.json` as we see fit.

I am claiming that this side-stepping of version control defined within
`Cargo.toml` is acceptable since we do not have any intention of publishing
the `query-planner-wasm` package to crates.io on its own.  Instead, we will
continue to publish the `wasm-pack`'d-from-this-crate
`@apollo/query-planner-wasm` package directly to npm's registry.  To reflect
that desire, I've marked the `query-planner-wasm` package as "private" using
the `private = true` property in its `Cargo.toml`.

In theory, this change could de-stabilize the work that `wasm-pack` does and
that's worth calling out.  For example, by not allowing it to generate the
`files` property (which indicates which artifacts are emitted into the `npm
pack`'d bundle), we might be not allowing it to add other (Future?
Unexpected?) important emitted files.  I've instead put those `files`
directly into the new `package.json` source of truth, along with other
appropriate properties, like `types` and `main`.  The largest risk here -
based on our intended use of the package - seems to be relevant only if we
were to rename the host Crate.  To demitigate that risk, I've explicitly
wired up the `package.json` with `--out-dir` and `--out-file` flags to
ensure that it always emits `index`-prefixed files.  Furthermore, they are
now emitted into the `dist` directory, to be parallel with all our other npm
package patterns which do the same.

I will note that, `wasm-pack` may address some of the work-arounds here in
the future, but best I can tell, they will be roughly compatible with what
we're doing here.  I am basing this outstanding issues, PRs, RFCs and
documentation on the `wasm-pack` project, referenced below.

Ref: https://rustwasm.github.io/rfcs/008-npm-dependencies.html#unresolved-questions (See last section)
Ref: https://rustwasm.github.io/docs/wasm-pack/commands/build.html#extra-options (See footnote)
Ref: rustwasm/wasm-pack#606
Ref: rustwasm/wasm-pack#840
@abernix abernix mentioned this issue Sep 23, 2020
Merged
abernix added a commit to apollographql/federation that referenced this issue Sep 24, 2020
@abernix @trevor-scheer
Setup CircleCI/Lerna/npm to compile and publish `@apollo/query-planne…
a32684c 
…r-wasm` (#181)

* Setup CircleCI/Lerna/npm to compile and publish `@apollo/query-planner-wasm`

This allows us to compile and depend upon the local version of the WASM
query planner package and get the tarballs that are packed with the right
version.  Additionally, this allows Lerna to handle the version bumping and
publishing for `@apollo/query-planner-wasm`, since it is uniquely positioned
to do that best in a monorepo orchestration that involves multiple npm
packages with relative `file:` dependencies on each other and coupled with
our existing needs for Lerna and publishing workflows we have built.

Technically speaking, this relieves the `Cargo.toml` from its responsibility
of managing the `version` and shifts that responsibility to `lerna` and
other npm-specific tooling we have connected to that.  In fact, this change
completely eliminates the need for the `wasm-pack` generated `package.json`
that is derived from the Cargo.toml (and rendered into the "out-dir",
previously `pkg/`).

While I would believe that there are some advanced `wasm-pack` cases where
this above change might be undesirable, for now, the constraint incurred by
that transposition is unblocking in other ways.  For example, rather than
necessitating parallel metadata support in the `Cargo.toml` for every
`package.json` property (think about npm specific properties like "labels",
"bugs", "private"), we can simply change our `package.json` as we see fit.

I am claiming that this side-stepping of version control defined within
`Cargo.toml` is acceptable since we do not have any intention of publishing
the `query-planner-wasm` package to crates.io on its own.  Instead, we will
continue to publish the `wasm-pack`'d-from-this-crate
`@apollo/query-planner-wasm` package directly to npm's registry.  To reflect
that desire, I've marked the `query-planner-wasm` package as "private" using
the `private = true` property in its `Cargo.toml`.

In theory, this change could de-stabilize the work that `wasm-pack` does and
that's worth calling out.  For example, by not allowing it to generate the
`files` property (which indicates which artifacts are emitted into the `npm
pack`'d bundle), we might be not allowing it to add other (Future?
Unexpected?) important emitted files.  I've instead put those `files`
directly into the new `package.json` source of truth, along with other
appropriate properties, like `types` and `main`.  The largest risk here -
based on our intended use of the package - seems to be relevant only if we
were to rename the host Crate.  To demitigate that risk, I've explicitly
wired up the `package.json` with `--out-dir` and `--out-file` flags to
ensure that it always emits `index`-prefixed files.  Furthermore, they are
now emitted into the `dist` directory, to be parallel with all our other npm
package patterns which do the same.

I will note that, `wasm-pack` may address some of the work-arounds here in
the future, but best I can tell, they will be roughly compatible with what
we're doing here.  I am basing this outstanding issues, PRs, RFCs and
documentation on the `wasm-pack` project, referenced below.

Ref: https://rustwasm.github.io/rfcs/008-npm-dependencies.html#unresolved-questions (See last section)
Ref: https://rustwasm.github.io/docs/wasm-pack/commands/build.html#extra-options (See footnote)
Ref: rustwasm/wasm-pack#606
Ref: rustwasm/wasm-pack#840

* Update query-planner-wasm/package.json

Co-authored-by: Trevor Scheer <[email protected]>

Co-authored-by: Trevor Scheer <[email protected]>
@geoknee geoknee mentioned this issue Aug 26, 2021
Open
@AmateurECE
Copy link

Is this an old issue? I was not able to recreate this issue using wasm-pack 0.10.3. I did see that the dependency would be optimized out if step #3 isn't completed in the "steps to reproduce" above.

@gadomski gadomski mentioned this issue Feb 18, 2025
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@bminixhofer @AmateurECE