-
Notifications
You must be signed in to change notification settings - Fork 773
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
Setup gradual process for migrating to Typescript #495
Conversation
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.
Yeah, really great that you are tackling this. Gradual process totally makes sense, already wondered on a high level how we should approach here but didn't put any structural thought into it yet.
Looks good, exited that this gets a start! 😄 🎊
} | ||
|
||
/** | ||
* bitwise or blooms together | ||
* @method or | ||
* @param {Bloom} bloom | ||
*/ | ||
or (bloom) { | ||
or (bloom: Bloom) { | ||
if (bloom) { | ||
for (let i = 0; i <= BYTE_SIZE; i++) { | ||
this.bitvector[i] = this.bitvector[i] | bloom.bitvector[i] |
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.
Looks good (the whole file), super-much a fan of making the API here more strict and restrict to Buffer
.
@@ -66,7 +66,7 @@ module.exports = class Memory { | |||
} | |||
} | |||
|
|||
const ceil = (value, ceiling) => { | |||
const ceil = (value: number, ceiling: number): number => { | |||
const r = value % ceiling | |||
if (r === 0) { | |||
return value |
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.
OK (file).
} | ||
|
||
return false | ||
} | ||
} |
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.
OK (file), yes, makes totally sense to inline this private method.
"async": "^2.1.2", | ||
"async-eventemitter": "^0.2.2", | ||
"core-js-pure": "^3.0.1", |
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.
Seems to be ok as a new dependency, see here.
|
||
b1.or(b2) | ||
st.true(b1.check('value 2'), 'should contain "value 2" after or') | ||
st.true(b1.check(utils.toBuffer('value 2')), 'should contain "value 2" after or') | ||
st.end() | ||
}) | ||
|
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.
OK, test adoption to the stricter Bloom
filter API.
"strict": true | ||
}, | ||
"include": ["./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.
Rest of the changes until here ok.
Side note 1: not mission-critical for the moment, just for the state of stuff: prettier/formatting is not working yet, or is it? Side note 2: let me know what concretely you are working on throughout the day, eventually (possible not, hmm 😛) I will also do one or two file transitions if I find the time. |
Well, the command runs, but I haven't fixed the errors :-D They were mostly on the
Sure! Until Monday I'll be working very sporadically and can't really plan it, but from Monday we can divide and conquer together ;) |
It obviously won't be possible to migrate the whole library to typescript at once. This PR sets up a process for migrating gradually. In the meanwhile JS and TS code will have to co-exist. To account for that:
babel
dependenciestsconfig.json
which allows JS code. All code is transpiled viatsc
(even JS)dist
) rather than source. This is until all source code has been migrated and can be later on revertedFor the first round, I've migrated
Bloom
,Stack
andMemory
(easier picks 😄):Bloom
's API has also been limited to only acceptBuffer
. Since this was only used internally, it shouldn't be a breaking changeStack
also only acceptsBN
(droppedBuffer
as it wasn't being used)rlp
as a dev dependency (due to version mismatch inethereumjs-util
, will make a PR for that)Please let me know if you think this gradual process is suitable or not.
(To be merged into #479)