-
Notifications
You must be signed in to change notification settings - Fork 137
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
SDK Modernization #584
SDK Modernization #584
Conversation
@BenRacicot would appreciate your review as well |
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.
Outstanding work! 💯 🚀
Couple questions...
Add 'prepare' script instead of lib/ folder
Fix GHAs to match build scripts
Makefile
Outdated
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 should be using the url for current:
https://github.com/stellar/stellar-core/tree/master/src/protocol-curr/xdr
for next:
https://github.com/stellar/stellar-core/tree/master/src/protocol-next
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 main xdr repository has moved out of core to https://github.com/stellar/stellar-xdr repo, curr
and next
branches respectively.
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 only came across one questionable thing which was the address to the xdr library...
Many of these changes are code linting, which is fine, but I'd prefer to see those broken into a separate commit to be able to quickly find the substantive changes.
Otherwise it looks good.
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.
that works, /generated
after all.
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.
LGTM, but I'm not an expert here.
Also, if you could merge these changes into the |
Epic: stellar/js-stellar-sdk#792
This is an overhaul to bring the
stellar-base
package to the modern JavaScript world.Summary
This involves a large set of changes, but I will do my best to summarize them to expedite review.
Main Changes
BigNumber.js
.Cascading Changes
These main changes motivate a cascade of other changes, broken down into a handful of categories:
yarn
scripts are differentBuilding
Webpack 5 support is here!
yarn build:node
yarn build:browser
cfg/webpack.config.browser.js
StellarBase
variableLinting
ESLint, Prettier, and DTSLint are all updated:
@babel/eslint-parser
in each.eslintrc.js
file and is implicitly part of building via Webpack (see above)@definitelytyped
project and has additional configuration implicitly as part of thetypes/.eslintrc.js
file which uses@typescript-eslint/parser
, insteadcfg/prettier.config.js
(andcfg/.prettierignore
) and runs on all files now. It's also (still) a pre-commit hook.Just run
yarn pretty && yarn lint
to format your files and check that everything is up to par.Testing
Istanbul: Istanbul is unmaintained and superceded by the
nyc
package.istanbul
plugin in.babelrc
tells Babel about itcfg/.nycrc
extends a Babel-compatible configuration file (docs)nyc
key inpackage.json
configures instrumentation behaviorbabel-plugin-istanbul
(docs), which is why"instrument": false
in the aboveNode Testing: Mocha (and its helper friends Sinon and Chai) has been updated.
mocha
key inpackage.json
configures Mocha (replacingmocha.opts
)nyc
(see above)Browser Testing: Karma has been updated.
nyc
(docs, see above)Run
yarn test
to run both suites.Scripts
Development has a slightly different workflow now:
yarn test:node
yarn build && yarn test:all
to get the both test suites (note that building first is required)yarn preversion
when you want a thorough, clean test of the whole system in "production" mode