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

develop: esm support #1936

Closed
wants to merge 52 commits into from
Closed

develop: esm support #1936

wants to merge 52 commits into from

Conversation

ryanio
Copy link
Contributor

@ryanio ryanio commented Jun 5, 2022

This PR adds ES modules support to develop, making it the primary module resolution method by adding "type": "module" to the package.jsons.

There is still some final work needed to complete all the pieces. Currently local tests are passing but karma is having some trouble, more details about karma in the bottom of this post.

This PR:

  • Makes dist a container folder with folders inside: esm, cjs, and types
  • Removes dist.browser build
    • We used to export separate browser builds to support ES5, but now that we are using native BigInts the browser builds also output ES2020, so there is no benefit or differentiation for a separate browser build.
  • Adds eslint rule node/file-extension-in-import to automatically add .js file extensions (really nice to have).
  • A couple node_modules have hotfix workarounds on postinstall scripts. For the ones I haven't opened issues on the respective repositories, would be nice to eventually do so the hotfixes can be removed in the future.
  • Sets esModuleInterop and allowSyntheticDefaultImports back to false (see here and Unnecessary enforcing esModuleInterop for users of the package  #1615)

package.json exports now looks like this:

  "main": "./dist/cjs/index.js",
  "types": "./dist/types/index.d.ts",
  "browser": "./dist/esm/index.js",
  "exports": {
    ".": {
      "import": "./dist/esm/index.js",
      "require": "./dist/cjs/index.js",
      "types": "./dist/types/index.d.ts",
      "browser": "./dist/esm/index.js"
    },
    "./*": {
      "import": "./dist/esm/*",
      "require": "./dist/cjs/*",
      "types": "./dist/types/*",
      "browser": "./dist/esm/*"
    }
  },

this allows for top-level exports, e.g.:

import VM from '@ethereumjs/vm
import Common, { Chain } from '@ethereumjs/common'

while also providing deep-access to files inside in case they are not exported at the top level, e.g.:

import Bloom from '@ethereumjs/vm/bloom/index.js'
import { GenesisState } from '@ethereumjs/blockchain/genesisStates/index.js'


I haven't been able to get the commonjs typescript build working yet since it complains about some of the esm changes, so one approach might be to instead build the commonjs version with webpack from the esm code. Additionally single file browser bundles could be added so others can easily use these packages from the browser:

  "exports": {
    ...
    "./esm-browser-bundle": "./dist/bundles/esm.js",
    "./iife-browser-bundle": "./dist/bundles/iife.js",
    "./umd-browser-bundle": "./dist/bundles/umd.js",
  }

@MicahZoltu shared his thoughts about bundling in discord:

I generally recommend against libraries doing bundling. Bundling should be done (IMO) at the application layer.
Under this paradigm, you would publish ESM and CJS builds of your library, and you wouldn't bundle in transitive dependencies.
The caveat here is that if you have any dependencies that don't provide ESM builds then things get complicated.
Ideally, you just don't have any non-ESM dependencies.
And if there is some dependency that you really need which doesn't have an ESM build you would submit a PR to add an ESM build and make the world a better place for everyone. 😀

I would agree here to keep our builds light and modern for the esm future without the burden of compiling the transitive dependencies. This would be aligned with our current strategy (on master branch) to let application layer do the bundling, since each set of needs could be different.

karma

I haven't been able to get karma fully working yet, getting this error on start: SyntaxError: import declarations may only appear at top level of a module. If we export browser bundles with e.g. webpack, then it might make sense to switch from karma-typescript to karma-webpack for better e2e testing.

(I only suggest webpack because we already use it in the client. If rollup is a better experience, then that can also be used and the client build can also be switched over to it to share the same deps.)

jochem-brouwer and others added 30 commits May 4, 2022 10:52
* vm: add codeHash db prefix to stateManager

* vm: tests: make tests use StateManager instead of trie at utilization
vm: tests: remove now passing test from skipTests

* vm: fix tests / fix runBlock/runBlockchain tests

* vm: tests: fix cleaning up untouched accounts in tests

* vm -> StateManager: added comment on CODEHASH_PREFIX

Co-authored-by: holgerd77 <Holger.Drewes@gmail.com>
Co-authored-by: steveluscher <github@steveluscher.com>
…orks to VM (#1649)

* common: remove onlySupported and onlyActive hf options

* vm: move supportedHardforks option from common

* common: remove supportedHardforks option

* common: update README for supportedHardforks removal

* vm: update README for supportedHardforks addition

* common: cleanup in src/index.ts

* vm: refactor supportedHardforks per PR feedback

* common: remove _chooseHardfork method per PR feedback

* vm: updated README per PR feedback

* vm: supportedHardforks local variable, switch to enum
* common: delete deprecated hardforkBlock method

* common: adapt tests to use hardforkBlockBN

* common: delete deprecated nextHardforkBlock method

* common/tests: rename nextHardforkBlock() calls
to nextHardforkBlockBN()

* common: adapt tests to use BN.eqn(number)

* common: delete deprecated networkId() method

* common: delete tests for networkId()

* common: delete deprecated chainId() method

* common: delete tests for .chainId()

* delete deprecated forCustomChain meethod

* common: delete forCustomChain() test

* blockchain: switch forCustomChain to Common.custom

* client: switch fromCustomChain to Common.custom

* tx: switch forCustomChain to Common.custom

* VM: switch forCustomChain to Common.custom

* common: internalize _getInitializedChains() method

* common: delete deprecaed chains/index.ts file

* client: switch to Common._getInitializedChains

* common: remove genesisStates/index.ts

* common: remove test for genesisStates
* common: rename nextHardforkBlockBN()
to nextHardforkBlock()

* common/tests: rename nextHardforkBlockBN()
to nextHardforkBlock()

* client: rename common.nextHardforkBlockBN()
to common.nextHardforkBlock()

* devp2p: rename common.nextHardforkBlockBN()
to common.nextHardforkBlock()

* common: rename hardforkBlockBN()
to hardforkBlock()

* common/tests: rename common.hardforkBlockBN()
to common.hardforkBlock()

* block: rename common.hardforkBlockBN()
to common.hardforkBlock()

* client: rename common.hardforkBlockBN()
to common.hardforkBlock()

* blockchain: rename common.hardforkBlockBN()
to common.hardforkBlock()

* devp2p: rename common.hardforkBlockBN()
to common.hardforkBlock()

* vm: rename common.hardforkBlockBN()
to common.hardforkBlock()

* common: rename chainIdBN()
to chainId()

* common/tests: rename common.chainIdBN()
to common.chainId()

* client: rename common.chainIdBN()
to common.chainId()

* blockchain: rename common.chainIdBN()
to common.chainId()

* devp2p: rename common.chainIdBN()
to common.chainId()

* tx: rename common.chainIdBN()
to common.chainId()

* vm: rename common.chainIdBN()
to common.chainId()

* common: rename networkIdBN() to networkId()

* common/test: rename common.networkIdBN()
to common.networkId()

* client: rename common.networkIdBN()
to common.networkId()

* block: rename common.networkIdBN()
to common.networkId()
* Replace BN.js with bigints in VM

* Fix byteLength computation in EXP handler

* Fix toTwos helper

* Compute TWO_POWE256 using math instead of hex

* Update packages/util/src/constants.ts

Co-authored-by: Ryan Ghods <ryan@ryanio.com>

* Remove unused variable

* Fix exponent byte length calc

* Fix exp overflow

* Fix precompile bigint conversions

* Fix more bigint conversions

* Fix EXP opcode handler

* Fix logic bug in signextend

* vm/gas: fix EXTCODECOPY gas

* vm/gas: fix sha256 gas

* vm/gas: sha256 const -> let

* vm: lint

* vm/logic: fix sdiv/smod and fromTwos/toTwos

* vm/logic: fix SIGNEXTEND

* vm/logic: fix CALLDATALOAD padding

* vm/logic: remove weird comment

* Fix SAR opcode handler

* Use bufferToBigInt in Push opcode handler

* use bufferToBigInt everywhere

* Fix missing bufferToBigInt import

* Check for edge case in expmod

* Ignore prettier

* Update browser tsconfig to es2020 lib

* Remove dup ES2020 targets

* attempt to dedupe "target" and "lib" tsconfig values

* Update karma config to target es2020 in parser

* Various test fixes

* Lint and BN fixes

* Add bigint helpers to util

* lint fixes

* various bigint helper additions

* Lint fixes

* Fix bnToBigInt

* Lint/test fixes

* Switch Xn to BigInt(X)

* lint

* lint

* More Xn to BigInt(X) moves

Co-authored-by: Ryan Ghods <ryan@ryanio.com>
Co-authored-by: Jochem Brouwer <jochem.brouwer@nslogin.nl>
Co-authored-by: Jochem Brouwer <jochembrouwer96@gmail.com>
* Tx: Remove baseTransaction.transactionType

* tx: remove get senderR()

* tx: remove test for senderR()

* tx: remove senderS()

* tx: remove test for senderS()

* tx: remove yParity / replace with v

* tx: remove test for yParity()

* Tx: remove fromRlpSerializedTx()

* Tx: remove test for fromRlpSerializedTx()

* Tx: remove _unsignedTxImplementsEIP155

* Tx: Remove _signedTxImplementsEIP155()

* Tx: Remove TransactionFactory.getTransactionClass

* Tx: Remove test for .getTransactionClass()
* common: set default hardfork to london

* vm: set default hardfork to london

* vm: update tests after default hardfork change

* vm: update default hardfork in tester config

* block: update default hardfork and fix tests

* blockchain: fix tests for london as default hardfork

* tx: set default hardfork to london

* block: fix for uncles at hardfork transition test

* blockchain: fix for hardfork in reorg test

* ethash: fix ethash tests for default hardfork change

* tx: remove README section about hardforks

* client: fix client tests for default hardfork change

* block: rename testdata files for pre-london

* blockchain: rename testdata files for pre-london

* vm: fix examples for London hardfork change

Co-authored-by: Emerson Macro <emersonmacro@gmail.com>
* common: rename hardforkIsActiveOnChain to isIncludedHardfork

* common: remove activeHardfork and activeHardforks

* common: adapt _getHardfork, remove isIncludedHardfork

* common: type fixes in forkHash
* add modifyAccountFields method and tests
* use new method in eei
* update usage in various tests
…er (#1765)

* client fix parse: now with common default being london, explicitly setHardforkByBlockNumber(0) when creating genesis header
* vm: move new modifyAccountFields to baseStateManager
* fix lint
* util: use bigints. Expose new secp constants

* common: use bigints

* tx: use bigints

* lint: fix util, tx, common

* tx: fix isSigned

* update comment block style to be picked up by vscode

* remove doc typo (extra `)

* tx: add isSigned() tests to base.spec.ts to iterate across txTypes

* block: bn to bigint changes wip

block: finish header changes

block: test updates

block: test fixes

Partial difficulty fixes

* block: fix homestead difficulty

* block: fix >= Byzantium difficulty

* BigInt conversion fixes

* block: update st.ok to st.equals

* Update readme to use bigints

* ethash: bn to bigint

* blockchain: wip bn to bigint changes

* devp2p: bn to bigint

* vm: wip remaining bn -> bigint

* vm: more test fixes

* fix runTx negative value args test, ensure balance does not go negative when using skipBalance

* vm: lint:fix

* re-add newline

* Blockchain: small rebase fix

* vm: Fix API tests

* client: bn to bigint updates in source

* client: various fixes

* client: last fixes

* client: integration test fixes

* replace st.ok usage with st.equal

* normalize st.equals to st.equal

* update toType cases

* nits, lint

* fix vm benchmarks

* client: fix fullEthereumService tests

* touch ups, fix miner integration test

* use prefix increment

* client: fix full sync test

* test fixes

* Fix reorg logic bug

* bnTo... function renaming and cleanup

* Goodbye bn.js

* reverse changelog changes

* nits

* remove more BN, more nits

* turn off restrict-plus-operands and remove disable overrides (does not work properly)

* more nits, update package-lock

* fix build errors, re-add @types/bn.js for rlp v2 export to fix build (will be removed in subsequent PR for rlp)

* replace miller-rabin (bn.js) with bigint-crypto-utils isProbablyPrime

* more nits / fixes

* Fix misplaced paren

* Fix miner test

* more nits

* fix buffer to big int

* set expectedTests back to >= comparison

* last fixes

* Removed re-introduced Common methods

Co-authored-by: Paul Miller <paul@paulmillr.com>
Co-authored-by: Ryan Ghods <ryan@ryanio.com>
Co-authored-by: acolytec3 <17355484+acolytec3@users.noreply.github.com>
Co-authored-by: Jochem Brouwer <jochembrouwer96@gmail.com>
Co-authored-by: ScottyPoi <scott.simpson@ethereum.org>
* blockchain: remove storing unsettled promise

* vm: remove storing unsettled promises. make copy() async

* client: update to new async changes

* block: doc nit

* nits

* fix vm benchmarks, add _init to mockchain

* Make blockchain and vm constructors protected

* lint

* vm: update new tests to await

Co-authored-by: acolytec3 <17355484+acolytec3@users.noreply.github.com>
* vm: v6 breaking changes

* update benchmark util to use gasUsed as bigint

* VM: fixed gasUsed rebase related bug

Co-authored-by: Holger Drewes <Holger.Drewes@gmail.com>
* add eip3074

* remove extra space

* use vm.create instead of new vm

Co-authored-by: Ryan Ghods <ryan@ryanio.com>
* util: move misplaced functions

* add unpadBuffer back

* Fix tttttttttttttypo

* make unpadHexString return Hex String!
* blockchain/tests: remove tests for getHead()

* vm: change blockchain.getHead()
to blockchain.getIteratorHead()

* vm: remove tests for blockchain.getHead()

* client: change blockchain.getHead()
to blockchain.getIteratorHead()

* blockchain: remove setHead()
for setIteratorHead()

* blockchain: edit text of
setIteratorHead() test

* blockchain: rename getLatestHeader()
to getCanonicalHeadHeader()

* blockchain/tests: rename getLatestHeader()
to getCanonicalHeadHeader()

* client: rename blockchain.getLatestHeader()
to blockchain.getCanonicalHeadHeader()

* client/tests: rename blockchain.getLatestHeader()
to blockchain.getCanonicalHeadHeader()

* blockchain/tests: rename getLatestBlock()
to getCanonicalHeadBlock()

* client: rename blockchain.getLatestBlock
to blockchain.getCanonicalBlockHeader()

* client/tests: rename blockchain.getLatestBlock
to blockchain.getCanonicalBlockHeader()

* blockchain: remove get meta()

* VM: Remove blockchain.meta from tests

* Blockchain: Remove blockchain.meta from tests

* blockchain: fix linting errors

* BN to BigInt misses from rebase

* BN to BigInt fixes + lint

* Fix missing name replacement

* lint

* Add back blockchain testrunner last header check

* Add test for call with no gas

* Add back correct renamed functions

* lint fixes

Co-authored-by: ScottyPoi <scott.simpson@ethereum.org>
* move ethash depBrowserWorkaround fix for bigint-crypto-utils to blockchain karma.conf.js
  (karma has trouble with the default browser esm import without es6 transform, so it is easier to tell it to use umd)

* new DefaultStateManager will init a trie if not supplied, so we don't need to init one here

* remove backwards compatibility note (we will keep this property)

* trie: remove old backwards compat methods

* client: do undefined check first

* vm: undo doc change

* client: improve receipt size calculation

* vm: resolve more todos

* vm: more hardfork enum usage

* dedupe receipt rlp encoding

* lint

* runCall, runCode: add more param typedocs, simplify

* also add karma workaround to vm
acolytec3 and others added 8 commits May 27, 2022 12:16
* common: create new common.ts

* common: finish reorganize/rename in common

* Address comments
* tx: edit legacyTransaction to throw when hash() called on unsigned tx

* tx: adapt legacy.spec test to throw on unsigned tx.hash()

* tx: edit tests which call hash() on unsigned tx
* simplify setting blockchan genesis parameters for future extensibility, make timestamp optional instead of `| null`

* move EOF one folder up, export default rather than wildcard import

* remove `assert` usage from vm/precompiles, devp2p

* eof: reinstate individual imports

* make genesisBlock a property and add a separate createGenesisBlock method

* fix mock, remove commented out code (double checked, geth does not return the eth protocol versions in admin_nodeInfo, so this is safely removable)

* remove old chain/hardfork check

* simplify errors, use increment operator, use for..of rather than forEach
* vm: update eip 3074

* vm: eip3074 test fixes start

* vm: fix eip3074 tests

* vm: expand eip3074 tests

* vm: add eip 3074 tests

* vm: address 128n -> BigInt(128)
@ryanio
Copy link
Contributor Author

ryanio commented Jun 6, 2022

Some more digging into karma, I thought these acorn options might help but no luck so far. Still getting Uncaught SyntaxError: Cannot use import statement outside a module. I suspect it's routing it through commonjs bundling instead of trying to use esm.

        acornOptions: {
          ecmaVersion: 11,
          sourceType: 'module',
          allowImportExportEverywhere: true,
        },

There's some lengthy information about the bundling here https://github.com/monounity/karma-typescript/tree/master/packages/karma-typescript#automatic-bundling and it says

However, this intentional behavior makes it possible to use karma-typescript for projects that use the Typescript module system, or have the compiler option module set to another value than commonjs, or simply put everything on the global scope.

So this should be supported.

@MicahZoltu
Copy link
Contributor

Why are the types separated from the module output? One issue with this is that the type definitions for cjs may not be the same as the type definitions for esm, so they generally shouldn't share type definition files. Also, it is a defacto standard to include TS definitions next to the output files and source maps, and following that defacto standard is more likely to result in stuff "just working" without the user needing to do additional work.

"module": "node16",
"moduleResolution": "node16",
"resolveJsonModule": true,
"sourceMap": true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Recommend declarationMap: true as well (https://www.typescriptlang.org/tsconfig#declarationMap).

Comment on lines -14 to +15
import { BlockHeader } from './header'
import { BlockData, BlockOptions, JsonBlock, BlockBuffer, Blockchain } from './types'
import { BlockHeader } from './header.js'
import { BlockData, BlockOptions, JsonBlock, BlockBuffer, Blockchain } from './types.js'
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than doing this, you could use a compiler plugin to automatically do it. There is much disagreement on what is "correct" here, but I'm of the opinion that you should leave ./header and let the compiler add the extension. The reason for this is because you aren't actually importing ./header.js, you are importing ./header.ts and then after transpilation it should turn into ./header.js.

If you want to have the compiler automatically add the extension, rather than adding them manually, you can use the extension here:
https://github.com/Zoltu/typescript-transformer-append-js-extension/

Copy link
Member

@holgerd77 holgerd77 Jun 7, 2022

Choose a reason for hiding this comment

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

Yes, I also have to say that I am still battling hard with accepting that this (the .js file extension) needs to be added (and I am not sure how this battle will play out 😋, since I am strongly feel this inconsistency Micah mentioned, so having these "wrong references" in our whole code base and TypeScript somewhat generously "looking over it" (does it?)).

So can we eventually give this extension a try?

@holgerd77
Copy link
Member

Why are the types separated from the module output? One issue with this is that the type definitions for cjs may not be the same as the type definitions for esm, so they generally shouldn't share type definition files. Also, it is a defacto standard to include TS definitions next to the output files and source maps, and following that defacto standard is more likely to result in stuff "just working" without the user needing to do additional work.

@MicahZoltu Could you very concretely outline what this would specifically mean so that we make sure that we are on the same page? Not using the types location reference in package.json? 🤔

@holgerd77
Copy link
Member

Some procedural note: this is still so highly invasive that I am not sure if we can merge this in early respectively "in time", so I would consider to not include this in the first round of beta releases for the breaking releases.

Since other work is waiting on a new develop branch basis I would still continue and replace develop with the rebased version once #1928 is ready, hope this is not causing too much hazzle regarding this PR.

@acolytec3
Copy link
Contributor

I've started poking around in this PR and I'm seeing a lot of errors I don't really understand trying to build it so not sure if that's what @ryanio was referring to by the CJS stuff not working but I'm guessing it'll take me some time to wrap my arms around it.

@acolytec3
Copy link
Contributor

I will also say I don't love the idea of adding a webpack build step to every one of our libraries. If the build step is half as slow as it is for our client build step at present, it will likely greatly increase the time it takes for an npm run clean && npm i refresh of the repo (which we have to do somewhat regularly at present).

@MicahZoltu
Copy link
Contributor

@MicahZoltu Could you very concretely outline what this would specifically mean so that we make sure that we are on the same page? Not using the types location reference in package.json? 🤔

The fix I am proposing would just involve removing the types keys from package.json and removing declarationDir key from tsconfig.json.

@ryanio
Copy link
Contributor Author

ryanio commented Jun 8, 2022

i've separated out tasks from this PR to #1946, will close this PR for now, @MicahZoltu pls take a look at the issue and feel free to suggest anything else in there.

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.