-
Notifications
You must be signed in to change notification settings - Fork 5k
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
ESM CJS builds #5904
ESM CJS builds #5904
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## 4.x #5904 +/- ##
==========================================
+ Coverage 82.14% 82.15% +0.01%
==========================================
Files 137 137
Lines 5981 5985 +4
Branches 1572 1576 +4
==========================================
+ Hits 4913 4917 +4
Misses 1068 1068
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Bundle StatsHey there, this message comes from a github action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
View detailed bundle breakdownAdded
Removed
Bigger No assets were bigger Smaller No assets were smaller Unchanged
|
Deploying with Cloudflare Pages
|
node users will need to use |
@@ -15,7 +15,8 @@ You should have received a copy of the GNU Lesser General Public License | |||
along with web3.js. If not, see <http://www.gnu.org/licenses/>. | |||
*/ | |||
|
|||
import { TransactionFactory, TypedTransaction } from '@ethereumjs/tx'; | |||
import { TypedTransaction } from '@ethereumjs/tx'; | |||
import defaultImport, * as fullImport from '@ethereumjs/tx'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While local testing in ESM app and importing web3 ESM build there was problem in importing TransactionFactory
from @ethereumjs/tx
, reason with ESM @ethereumjs/tx
is not exposing named export but exporting under default but with CJS @ethereumjs/tx
is exporting it with named export. Following fix is applied in eth and accounts package and it imports both default and all ( named + default ) making sure that we will not miss TransactionFactory
in either esm or cjs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This export problem is fixed in @ethereumjs/tx in 4.x and we can upgrade to that in our 4.x after reviewing breaking changes in another PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm maybe you should try again, now that types are first in export. Maybe also publish repro repo^^
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It isnt issue in web3 exporting, but in ethereumjs/tx lib. I tested in another proj as well.
@@ -3,8 +3,6 @@ | |||
"resolveJsonModule": true, | |||
"forceConsistentCasingInFileNames": true, | |||
"target": "es2016", | |||
"module": "commonjs", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now we have base config for building in root with common configuration items.
@@ -0,0 +1,9 @@ | |||
{ | |||
"extends": "../../tsconfig.base.json", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
each package has three configs for building CommonJS, ESM and type defs, these are extending from base config in root
"build": "tsc --build", | ||
"build": "yarn build:cjs && yarn build:esm && yarn build:types", | ||
"build:cjs": "tsc --build tsconfig.cjs.json && echo '{\"type\": \"commonjs\"}' > ./lib/commonjs/package.json", | ||
"build:esm": "tsc --build tsconfig.esm.json && echo '{\"type\": \"module\"}' > ./lib/esm/package.json", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was explicitly required to have package json in build dir ( lib ) to explicitly override module type for correctly loading ESM when required so I am generating here a package json in lib
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you need that for cjs dir
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By default its not required, but I kept it to always override any higher level package config ( in client app) for surely giving error in case if any module type is loaded / referred by mistake.
Do you think it will cause any issue in client side if we keep this explicit overriding file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No idea tbh
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jdevcs Maybe it would be better to use nodejs script for this. Afaik this excludes developers on windows to build library^^
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for creating package.json? I think echo
is also avalible in win
"build": "tsc --build", | ||
"build": "yarn build:cjs && yarn build:esm && yarn build:types", | ||
"build:cjs": "tsc --build tsconfig.cjs.json && echo '{\"type\": \"commonjs\"}' > ./lib/commonjs/package.json", | ||
"build:esm": "tsc --build tsconfig.esm.json && echo '{\"type\": \"module\"}' > ./lib/esm/package.json", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was explicitly required to have package json in build dir ( lib ) to explicitly override module type for correctly loading ESM when required so I am generating here a package json in lib
"build": "tsc --build", | ||
"build": "yarn build:cjs && yarn build:esm && yarn build:types", | ||
"build:cjs": "tsc --build tsconfig.cjs.json && echo '{\"type\": \"commonjs\"}' > ./lib/commonjs/package.json", | ||
"build:esm": "tsc --build tsconfig.esm.json && echo '{\"type\": \"module\"}' > ./lib/esm/package.json", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you need that for cjs dir
@@ -14,12 +14,14 @@ GNU Lesser General Public License for more details. | |||
You should have received a copy of the GNU Lesser General Public License | |||
along with web3.js. If not, see <http://www.gnu.org/licenses/>. | |||
*/ | |||
import { TransactionFactory } from '@ethereumjs/tx'; | |||
import defaultImport, * as fullImport from '@ethereumjs/tx'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doing this doesn't increase the build size?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it shouldn't, because there is one copy of this dependency in node_modules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jdevcs But would this affect tree shaking since we're now importing everything and setting it to a const
instead of just importing and using the TransactionFactory
export?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this may not affect treeshaking as in case of ESM usage of our lib , dependency usage tree is still construable for module being used.
import defaultImport, * as fullImport from '@ethereumjs/tx';
const { TransactionFactory } = defaultImport || fullImport;
Even if any issue arise , the lib under discussion is decided to be removed, or we can upgrade this to latest @ethereumjs/tx where these exports are fixed as mentioned earlier:
This export problem is fixed in @ethereumjs/tx in 4.x and we can upgrade to that in our 4.x after reviewing breaking changes in another PR.
Description
#5241
Type of change
Checklist for 1.x:
npm run dtslint
with success and extended the tests and types if necessary.npm run test:cov
and my test cases cover all the lines and branches of the added code.npm run build
with success.dist/web3.min.js
in a browser.CHANGELOG.md
file in the root folder.Checklist for 4.x:
yarn
successfullyyarn lint
successfullyyarn build:web
successfullyyarn test:unit
successfullyyarn test:integration
successfullycompile:contracts
successfullyCHANGELOG.md
file in the packages I have edited.