-
Notifications
You must be signed in to change notification settings - Fork 5.1k
Fix Build Command (supersedes #4848) #4861
Conversation
|
Your Render PR Server URL is https://web3-js-pr-4861.onrender.com. Follow its progress at https://dashboard.render.com/static/srv-c8pe78nd17cafaqrv75g. |
Pull Request Test Coverage Report for Build 2002666498
💛 - Coveralls |
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.
We should not make changes in package-lock.json in this PR, as scope of issue is for improvement in build commands
|
@jdevcs this PR introduces a new dependency on |
Co-authored-by: Junaid <[email protected]>
| "mocha": "^6.2.3", | ||
| "nyc": "^14.1.1", | ||
| "pify": "^4.0.1", | ||
| "rimraf": "^3.0.2", |
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.
| "rimraf": "^3.0.2", | |
| "test:e2e:ganache:core": "bash ./scripts/e2e.ganache.core.sh", | ||
| "test:e2e:gnosis:dex": "bash ./scripts/e2e.gnosis.dex.sh", | ||
| "ci": "bash ./scripts/ci.sh", | ||
| "cov:clean": "rimraf .nyc_output && rimraf coverage", |
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.
| "cov:clean": "rimraf .nyc_output && rimraf coverage", | |
| "cov:clean": "rm -rf .nyc_output; rm -rf coverage", |
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.
The suggested change actively reduces cross-platform compatibility. Can you explain why it's important to avoid any changes to package-lock?
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.
@wbt We include changes in package-lock files via dedicated and internally initialized PRs only.
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 is now formally an internally initialized PR. Going out of your way to reduce cross-platform compatibility and break the command on some platforms just doesn't seem like the right direction.
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.
@wbt We include changes in package-lock files via dedicated and internally initialized PRs only.
Internal and also dedicated PRs for package lock changes. Thanks
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.
However, the version of this PR which does not change all the package-lock files this one does was rejected at #4848. In theory, I believe that one could be reopened and merged, but @spacesailor24 seems to prefer the version with all the additional package-lock changes while @jdevcs strongly disagrees, and meanwhile the command nominally required to be run for every PR remains broken.
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.
@wbt , we discussed this internally that @spacesailor24 will re generate package lock files in this PR, but there are errors when he tried. #4861 (comment)
Our team is occupied in 4.x so could you investigate and share it.
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.
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.
Its already merged so couldn't reopen.
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.
Replaced with #4939.
jdevcs
left a comment
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 this PR introduces a new dependency on
rimraffor therm -rffunctionality, so package-lock.json does need to be on the changed files list.
@spacesailor24 we need to remove changes from package lock files in this PR. thanks
avkos
left a comment
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.
rimraf works exactly the same as rm-rf but with cross-platform support. Why do we need to change rimraf to rm -rf? What is it solve?
@avkos Web3.js supports Windows, OSX, and Linux environments, hence the cross platform support needed for removing files |
|
Okay so I deleted all the ../../node_modules/@ethereumjs/common/dist/types.d.ts(38,32): error TS1005: ',' expected.
../../node_modules/@ethereumjs/common/dist/types.d.ts(38,58): error TS1005: ',' expected.
../../node_modules/@ethereumjs/common/dist/types.d.ts(40,12): error TS1005: ',' expected.
../../node_modules/@ethereumjs/common/dist/types.d.ts(41,9): error TS1005: ',' expected.
../../node_modules/@ethereumjs/common/dist/types.d.ts(42,12): error TS1005: ',' expected. |
I mean |
Original PR: #4848, this fixed some merge conflicts with
package-lock.jsonSomething to note is a ran
npm iwhich apparently fixed the version number of all packages, changing them from1.7.1-rc.0to1.7.1