Skip to content

Add nitro-protocol dependency to packages/wasm-utils manifest during bootstrap #29

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

Merged
merged 6 commits into from
Sep 13, 2021

Conversation

geoknee
Copy link
Contributor

@geoknee geoknee commented Sep 6, 2021

See #28 .

The generated npm package depends on nitro-protocol (you can see this by inspecting the gitignored file /packages/wasm-utils/wasm_utils.d.ts). That dependency is missing from the manifest of the generated npm package, which causes an error to be thrown in consuming projects.

Changes [Optional]

  1. Add dependency to the checked-in file /native-utils/wasm-utils/package.json. Ideally this would be sufficient to make it appear in native-utils/packages/wasm-utils/package.json. I think in the end it is neither necessary no sufficient, so I undid this change.
  2. Append a script to the prepare step in /native-utils/wasm-utils/package.json, which has the desired effect of adding the missing dependency.
  3. Upgraded wasm-pack

How Has This Been Tested?

I have performed a manual test that implies this fix works.

I have a branch in our main monorepo where server-wallet depends on a version of nitro-protocol pulled from npm, whereas other packages such as wallet-core depend on the local version. This leads to the npm version of nitro-protocol being installed under /packages/server-wallet/node_modules (i.e. it is not hoisted by yarn). On the other hand, wasm-utils is hoisted and therefore resides at /node_modules. In this scenario, wasm-utils cannot find nitro-protocol due to the missing dependency. I yarn linked that branch (locally) to this one, and was able to prepare the server-wallet even when I had deleted the local nitro-protocol build. nitro-protocol was installed under /node_modules/@statechannels/wasm-utils/node_modules.

With an unlinked wasm-utils, this was not possible and I saw

➜  server-wallet git:(integrate-exit-format/wallet-core-rebased) yarn prepare
yarn run v1.22.5
$ rm -rf lib; tsc -b . && node ./scripts/inject-wallet-version.js
../../node_modules/@statechannels/wasm-utils/wasm_utils.d.ts:53:32 - error TS2307: Cannot find module '@statechannels/nitro-protocol' or its corresponding type declarations.

53 import { Channel, State } from '@statechannels/nitro-protocol';
                                  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Note that doing yarn prepare from the top level of the monorepo can lead to a race condition where this problem wouldn't always show up. If the local nitro-protocol was built before the server-wallet (it is not a dependency in this scenario), then wasm-utils would find that and fail to complain. Otherwise, we see the error https://app.circleci.com/pipelines/github/statechannels/statechannels/13764/workflows/a5c2d7a7-21ca-4773-a422-03cde87bf380/jobs/66998 .


Checklist:

Code quality

  • I have written clear commit messages
  • I have performed a self-review of my own code
  • This change does not have an unduly wide scope
  • I have separated logic changes from refactor changes (formatting, renames, etc.)
  • I have commented my code wherever necessary (can be 0)
  • I have added tests that prove my fix is effective or that my feature works, if necessary

Project management

What matters is the dependencies specified in the generated npm package, not this one.
I had thought that specifying them here would result in them being in the manifest of the generated package. But that is not the case. Therefore it is unecessary to specify here.
@andrewgordstewart
Copy link
Contributor

Should we be bothering to keep native-utils/wasm-utils in sync? Can we remove the dependency on wasm-utils to avoid this work?

@geoknee
Copy link
Contributor Author

geoknee commented Sep 7, 2021

Should we be bothering to keep native-utils/wasm-utils in sync?

This change is not about keeping native-utils or wasm-utils in sync with anything. It's just about fixing something that is badly broken with these packages. That happens to unstick some work in our monorepo.

Can we remove the dependency on wasm-utils to avoid this work?

Yes it would be easy to do that. Reasons not to do that, and not to avoid this work:

  1. It's already done :-P
  2. It would leave the packages in this repo broken.
  3. It would count as a feature regression in server-wallet, building up more unnecessary technical debt.

So it seems preferable (to me) to fix the problem in this manner?

@andrewgordstewart
Copy link
Contributor

This is is a higher level question that is not specific to this PR, but I've observed a bunch of extra work being done due to the circular dependency between nitro-protocol and wasm-utils.

wasm-utils serves two purposes:

  1. it provides some (nice) CPU optimization
  2. it serves as a seed for a rust wallet implementation

If we're spending a lot of cycles dealing with wasm-utils, and 1 & 2 are not important, then shouldn't we break the circular dependency?


This is not an argument that we should remove wasm-utils. It is simply questioning whether wasm-utils is worthwhile to keep in sync with nitro-protocol.

@geoknee
Copy link
Contributor Author

geoknee commented Sep 7, 2021

If we're spending a lot of cycles dealing with wasm-utils, and 1 & 2 are not important, then shouldn't we break the circular dependency?

I understand where you're coming from. I think the amount of work is actually pretty low at the moment, in fact.

That said, I don't feel strongly about keeping wasm-utils around.

How about we merge this PR, and then revisit that question?

@andrewgordstewart
Copy link
Contributor

How about we merge this PR, and then revisit that question?

I'd lazily revisit that question the next time wasm-utils becomes a time sink.

geoknee added a commit to statechannels/statechannels that referenced this pull request Sep 8, 2021
This should have the same effects as statechannels/native-utils#29 (when that eventually gets reviewed, merged, and published to a new version of wasm-utils)
Copy link

@NiloCK NiloCK left a comment

Choose a reason for hiding this comment

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

The changes make enough sense to me, but the overall build and linking process here is pretty convoluted.

Attempts to build the generated package locally have failed (Thanks George!)

@geoknee geoknee merged commit a784338 into master Sep 13, 2021
@geoknee geoknee deleted the add-nitro-dependency branch September 13, 2021 08:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants