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

Issue running ergo-lib-wasm-browser in Node environments #632

Closed
mkermani144 opened this issue Sep 28, 2022 · 6 comments · Fixed by #637
Closed

Issue running ergo-lib-wasm-browser in Node environments #632

mkermani144 opened this issue Sep 28, 2022 · 6 comments · Fixed by #637

Comments

@mkermani144
Copy link
Contributor

Although it may seem weird to run ergo-lib-wasm-browser in Node environments (as we have ergo-lib-wasm-node), it makes sense when we consider writing tests for apps that use ergo-lib-wasm-browser using tools running in Node. It's not possible to write tests using Jest or Vitest, for example, if we add this package to an app dependencies.

The issue is not directly related to sigma-rust, however; Unfortunately, wasm-pack output package.json only contains a module field. There are no type: module, main or exports fields available so that Node can handle the package as expected. There is an open pull request in wasm-pack repo to fix the issue, but it's not merged yet.

Meanwhile, we can wait for wasm-pack to fix their issue and use tools such as patch-package to bypass the problem, or we can apply some minor modifications to the output of wasm-pack and add required fields to the generated package.json.

@greenhat
Copy link
Member

Thanks for bringing this up!
I have no high hopes that the wasm-pack PR will be merged anytime soon. We have a few scripts already that modify package.json after the build (see https://github.com/ergoplatform/sigma-rust/tree/develop/bindings/ergo-lib-wasm/scripts). Could you please add a new one for this matter?

@mkermani144
Copy link
Contributor Author

Sure.

I can add the official Node standard exports field, or I can just remove the module field in favor of main and type: module, like the open pull request in wasm-pack, so that we remove the script later IF the pull request gets merged. We can also just add new fields and leave the old ones as-is.

Which one do you think is better to implement?

@greenhat
Copy link
Member

Thanks for picking it up! I don't know that much about the ecosystem so I leave it up to you. I'm not sure if wasm-pack PR gets merged at all. I'd prefer the method that does not affect/brake the browser workflow.

@mkermani144
Copy link
Contributor Author

Ok.

I work on it and create a PR asap.

@ross-weir
Copy link
Collaborator

Another possible option could be using the node package in tests and the browser package in browser - since wasm-pack is targeting a different environment I wouldn't be surprised if there were other env related issues aside from imports.

I've done something like this in the past, and I think I've seen some other projects do something like this too:

export const initErgo = async (): Promise<Ergo> => {
  if (!ergo) {
    // Tests are ran using jest (nodejs) otherwise we should be in the browser
    ergo = await (process.env.NODE_ENV === 'test'
      ? import('ergo-lib-wasm-nodejs')
      : import('ergo-lib-wasm-browser'));
  }

  return ergo;
};

@mkermani144
Copy link
Contributor Author

@ross-weir
In my opinion, different JS environments are getting closer to each other. ESM support is a good example. I don't know if we get other env related issues in the future. In my own use case, that didn't happen, but it may happen for others.

Anyway, using the node version of the package instead of the browser version for tests is a kind of mocking. Although it works, I really prefer not to mock the whole package in favor of another package. Considering they both get generated from the same rust code, there still may be some differences in the generated code that we lose by mocking.

Mocking is a code smell. I think in our case, we can provide the better solution of fixing package.json and let the possible users facing other env related issues (if any) mock the package using the node output.

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

Successfully merging a pull request may close this issue.

3 participants