ethers-io / ethers.js

Complete Ethereum library and wallet implementation in JavaScript.
https://ethers.org/
MIT License
7.91k stars 1.83k forks source link

Cannot be bundled for target `node` with webpack #1108

Closed levino closed 1 year ago

levino commented 3 years ago

See https://github.com/ethers-io/ethers.js/issues/1083 for details. Feel free to reopen #1083 and close this one here.

samajammin commented 3 years ago

I'm having the same issue.

package.json

"ethers": "^5.0.17",

node file:

const { ethers } = require("ethers")
console.log(ethers)

errror:

'{"errorMessage":"window is not defined","errorType":"ReferenceError","stackTrace":...}'
samajammin commented 3 years ago

FYI - rolling back my version fixed the issue.

package.json

"ethers": "^4.0.47",
ricmoo commented 3 years ago

I’m working on overhauling how the ESM version gets generated starting today, which should address this too. :)

levino commented 3 years ago

Please first just remove the file from the build and also remove the module entry from the package.json which should have very little negative impact. Then publish and do your work and publish again once the file is fixed.

ricmoo commented 3 years ago

Remove which file? I can’t remove those of change the package.json fields without breaking other people.

levino commented 3 years ago

If the module field is missing any bundler will just take the main fields value. I think the only problem might be that projects will have larger bundle size because tree shaking wont work as well any more.

levino commented 3 years ago

See #1119

levino commented 3 years ago

On another note: Why are the build files committed to the codebase? I think they should only be generated locally before a publication, not be part of the git repo.

ricmoo commented 3 years ago

There are to many different bundlers and projects that consume ethers in various ways, I don’t think I feel comfortable changing the package fields right now.

I’m working on better bundling for ESM anyways. :)

ricmoo commented 3 years ago

The build files are committed for both personal preference and for security reasons.

For security, it is possible for the deterministic build in the CI to verify the generated files match the source files (I haven’t ported that test from v4 yet, but will soon). It also simplified diff-ing two different versions of the final library which makes incremental code auditing easier.

For personal preference, I often need access to dist files, which are often not included, which requires spinning up a secure environment just to build the files (npm install is generally not something you should be running on a non-secured host). Not everyone who wants to look at the dist files have the necessary build environment, and if they are sitting there they are much easier to browse on GitHub.

levino commented 3 years ago

How about just committing them to a different branch? Like a github-pages branch for the generated files? Should do the same trick.

levino commented 3 years ago

There are to many different bundlers and projects that consume ethers in various ways, I don’t think I feel comfortable changing the package fields right now.

I’m working on better bundling for ESM anyways. :)

My personal experience tells me, that if this fix is subjected to "general work to overhaul things", it will take ages to be shipped. I use patch-package until then to fix the package.json locally in the node_modules folder, like I did in #1119 ...

ricmoo commented 3 years ago

But why not commit them to the same branch then? Then it is obvious what sources they came from.

levino commented 3 years ago

As you said, the question on whether to "commit or not commit" is a personal preference. Your project, your rules... I never commit a generated file. I also do not build my packages on a "secure system". I think that is pointless if I use 100 dependencies built on whatever virus infested windows machines. But it does not hurt either...

ricmoo commented 3 years ago

This should be addressed in 5.0.20. Please try it out and let me know if you have any more issues. :)

levino commented 3 years ago

Could you please link the code change / commit too?

ricmoo commented 3 years ago

Here is the change: https://github.com/ethers-io/ethers.js/commit/22bd0c76dddef7134618ec70ac1b084a054e616e#diff-867fc130b92f9b215a2f56cb64caf802367a214d62de58b7688b4c56a3893e90

It was part of an overall refactoring of ESM generation, so was not tagged specifically in the commit.

levino commented 3 years ago

Sorry, I added more info in the PR. Here it is again:

Fix does not work. It changes things a bit, but ultimately results in:

2020-11-20T06:55:12.324 [Error] Error: could not detect network (event="noNetwork", code=NETWORK_ERROR, version=providers/5.0.15)

when using the library (executing code).

Again it looks to me like there is browser targeted code being loaded in a node environment. I repeat for clarity: I am bundling the package with webpack in order to run it with NodeJS on a server, not a browser. This makes sense and is legitimate. All other libraries support this.

I suggest to add a regression test which reproduces this issue before we continue the work on this. It takes me quite a time to install the package, build the software, ship and run it for testing. I would rather prefer if a robot would do that.

The test should go in the direction of:

That might sound like a lot of work, but it is necessary. The alternative would be to stop producing and shipping these fancy esm modules. Unfortunately at the moment it is done incorrectly.

levino commented 3 years ago

Also see this comment https://github.com/ethers-io/ethers.js/issues/1083#issuecomment-709353361

ricmoo commented 3 years ago

I don’t have time to try this out tonight, but can you try setting your mainFields to [ "main" ]. I just looked up some webpack defaults, and it seems odd that the node default for webpack mainFields uses "module" first. They really need module to be broken into two, something akin to "module” and "module.browser", but unfortunately the field was something various projects just started using without any formalizing... :(

Fixing this by modifying the package.json for webpack+node will break rollup and similar bundlers which rely on module being for the browser. I am still researching ways to solve this more generically (without the methods that massively inflate web builds)...

levino commented 3 years ago

I think you can and should use this solution https://github.com/rollup/rollup-plugin-node-resolve/issues/8#issuecomment-257323750

levino commented 3 years ago

Your assumption that it is okay in rollup-land to point module to a browser specific version of your software is incorrect. The module field is only for esm modules and indicates nothing about the environment in which they will be executed.

ricmoo commented 3 years ago

The problem with relying on "browser" is that it doesn’t get pulled in by default in environments where it is difficult to specify "mainFields". I tried this method originally and it was problematic as well.

Exactly, "module" doesn’t specify browser or not. So, one of those environments must be chosen. The majority of people using bundlers are targeting browsers, who it is also more important that tree-shaking is as meaningful as possible.

There are three possible field combinations available in package.json (browser, module and main), and 4 possible outputs (ESM+node, ESM+browser, es3+node, es3+browser), so one is going to have to be compromised, so it makes sense to be a bit harsher on the less-common/easier-to-work-around targets.

Does setting the mainFields to just "main" work?

I can look into the jsnext:main which I’m guessing points to ESM in node? They can point to the lib._esm which is currently discarded as it is only an intermediate step between TypeScript and the lib.esm... Is it widely supported by bundlers?

ricmoo commented 3 years ago

Looks like it is deprecated: https://gist.github.com/etaletai13/c1c3f47ec89459dfb3e049975115165c#jsnextmain

levino commented 3 years ago

Who is @etaletai13 and this file is 2 years old...

Detoo commented 3 years ago

can confirm that setting mainFields to ["main"] works in our node environment 🎉

levino commented 3 years ago

can confirm that setting mainFields to ["main"] works in our node environment

I do not understand. Could you be more verbose on your workaround?

ricmoo commented 3 years ago

It was the top link in DuckDuckGo. I’m just trying to find info on these alternate properties...

ricmoo commented 3 years ago

See: https://webpack.js.org/configuration/resolve/#resolvemainfields

But set the mainFields to [ "main" ].

Detoo commented 3 years ago

can confirm that setting mainFields to ["main"] works in our node environment

I do not understand. Could you be more verbose on your workaround?

we literally just added the following lines to webpack.config.js and everything was back to normal:

resolve: {
  mainFields: ["main"]
}
levino commented 3 years ago

So one could set mainFields in the webpack configuration to use only "main" and not ["module","main"]. But that would just cripple my whole project just to fix an issue from this package. I have a workaround in place with patch-package as I said and this patch just kicks out the line with the "module":... from the package.json of ethers.

It is simply wrong to have the module field in the package.json point to browser-specific code. That is an illegal usage of this field. I cannot find any place where someone says that this is okay. Use the browser field for this. The value can be a map, as I explained above.

levino commented 3 years ago

can confirm that setting mainFields to ["main"] works in our node environment

I do not understand. Could you be more verbose on your workaround?

we literally just added the following lines to webpack.config.js and everything was back to normal:

resolve: {
  mainFields: ["main"]
}

Your bundle size probably has exploded. Now all the other dependencies that you are using will get bundled with their umd modules even if they have correctly implemented and shipped nice and shiny esm modules.

levino commented 3 years ago

Please remove the label "fixed". Also I insist that this is a "bug" not an "enhancement".

Detoo commented 3 years ago

can confirm that setting mainFields to ["main"] works in our node environment

I do not understand. Could you be more verbose on your workaround?

we literally just added the following lines to webpack.config.js and everything was back to normal:

resolve: {
  mainFields: ["main"]
}

Your bundle size probably has exploded. Now all the other dependencies that you are using will get bundled with their umd modules even if they have correctly implemented and shipped nice and shiny esm modules.

I don't see much size difference in this specific case (2.52MiB -> 2.64MiB), but I don't know exactly how it works. Just'd like to provide a sample

EmCeeEs commented 3 years ago

So one could set mainFields in the webpack configuration to use only "main" and not ["module","main"]. But that would just cripple my whole project just to fix an issue from this package. I have a workaround in place with patch-package as I said and this patch just kicks out the line with the "module":... from the package.json of ethers.

It is simply wrong to have the module field in the package.json point to browser-specific code. That is an illegal usage of this field. I cannot find any place where someone says that this is okay. Use the browser field for this. The value can be a map, as I explained above.

As of version ethers@5.0.22 a single patch for ethers is not enough. Another package seems to cause the issue now. Although not ideal, we decided to go with @Detoo's solution by setting mainFields: ["main"] in webpack to fix it the issue.

0x62 commented 3 years ago

This is still an issue for me on 5.4.6, getting could not detect network and missing response for all calls when using Webpack and Cloudflare Workers. Constructing the request manually or running directly doesn't have the issue.

qtiki commented 2 years ago

I just ran into this issue. For those (like me) not wanting to modify the mainFields as it could adversely affect other modules you can define an alias for the package. Note that since there are actually a lot of packages in the @ethersproject namespace that have the same problem you'll need to list all of them:

{
  ethers: 'ethers/lib',
  '@ethersproject/abi': '@ethersproject/abi/lib',
  '@ethersproject/abstract-provider': '@ethersproject/abstract-provider/lib',
  '@ethersproject/abstract-signer': '@ethersproject/abstract-signer/lib',
  '@ethersproject/address': '@ethersproject/address/lib',
  '@ethersproject/base64': '@ethersproject/base64/lib',
  '@ethersproject/basex': '@ethersproject/basex/lib',
  '@ethersproject/bignumber': '@ethersproject/bignumber/lib',
  '@ethersproject/bytes': '@ethersproject/bytes/lib',
  '@ethersproject/constants': '@ethersproject/constants/lib',
  '@ethersproject/contracts': '@ethersproject/contracts/lib',
  '@ethersproject/hash': '@ethersproject/hash/lib',
  '@ethersproject/hdnode': '@ethersproject/hdnode/lib',
  '@ethersproject/json-wallets': '@ethersproject/json-wallets/lib',
  '@ethersproject/keccak256': '@ethersproject/keccak256/lib',
  '@ethersproject/logger': '@ethersproject/logger/lib',
  '@ethersproject/networks': '@ethersproject/networks/lib',
  '@ethersproject/pbkdf2': '@ethersproject/pbkdf2/lib',
  '@ethersproject/properties': '@ethersproject/properties/lib',
  '@ethersproject/providers': '@ethersproject/providers/lib',
  '@ethersproject/random': '@ethersproject/random/lib',
  '@ethersproject/rlp': '@ethersproject/rlp/lib',
  '@ethersproject/sha2': '@ethersproject/sha2/lib',
  '@ethersproject/signing-key': '@ethersproject/signing-key/lib',
  '@ethersproject/solidity': '@ethersproject/solidity/lib',
  '@ethersproject/strings': '@ethersproject/strings/lib',
  '@ethersproject/transactions': '@ethersproject/transactions/lib',
  '@ethersproject/units': '@ethersproject/units/lib',
  '@ethersproject/wallet': '@ethersproject/wallet/lib',
  '@ethersproject/web': '@ethersproject/web/lib',
  '@ethersproject/wordlists': '@ethersproject/wordlists/lib'
}

One option would be to write your own resolve plugin to handle this based on the namespace or if your Webpack config is in a .js file you could programmatically generate this list. That way it would be more future proof if/when new packages are added. However I'm pressed for time so this going to be our solution for the foreseeable future.

peetzweg commented 2 years ago

@qtiki Struggled half a day with this error, kudos for supplying this list with your solution. 🙏

ricmoo commented 1 year ago

This has been fixed in v6, which uses a straight-forward, no-weirdness build process. The list files are designed for the web, but all other files can be safely included as-is and your bundler will know what to do.

Closing this now. If you have any further issues though, please let me know.

Thanks! :)