-
Notifications
You must be signed in to change notification settings - Fork 760
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
Add browser field to package.json's of modules with browser builds #921
Conversation
Codecov Report
Flags with carried forward coverage won't be shown. Click here to find out more. |
@@ -1,6 +1,6 @@ | |||
{ | |||
"extends": "@ethereumjs/config-typescript/tsconfig.browser.json", | |||
"include": ["src/**/*.ts"], | |||
"include": ["src/**/*.ts", "src/**/*.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.
These jsons
were accidentally missing from the tsc browser config.
The errors looked like:
20 10 2020 15:04:25.170:ERROR [framework.browserify]: bundle error
20 10 2020 15:04:25.172:ERROR [framework.browserify]: Error: Can't walk dependency graph: Cannot find
module './mainnet.json' from '/ethereumjs-vm/packages/common/dist.browser/chains/index.js'
required by /ethereumjs-vm/packages/common/dist.browser/chains/index.js
...and the stack trace shows karma/browserfiy pulling files from dist.browser
so that's 👍 .
Will push some additional commits here in the next 15-20 min, please don't merge or rebase in between. |
d66ed1f
to
4a1678e
Compare
@@ -5,8 +5,10 @@ | |||
"main": "dist/index.js", | |||
"types": "dist/index.d.ts", | |||
"files": [ | |||
"dist" | |||
"dist", | |||
"dist.browser" |
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.
dist.browser
fields were missing here, added these in 4f33ced
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.
Ah!!! Good catch!
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.
@cgewecke Thanks! 😄
These browser builds should actually be available, added these on two subsequent commits also for blockchain
and vm
.
Before we release anything here we should actually do at least manual checks on all build types (node + browser) for all the packages - so minimally do a distribution, import from the respective dist
or dist.browser
directory, do an instantiation and execute at least one function - or alternatively integrate something like this into CI.
Addresses leftover review comment from #913/886.
Follows pattern established in merkle-patricia-tree 117.
Per clarifying comment there:
There are 4 packages with browser builds: