Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Setup gradual process for migrating to Typescript #495

Merged
merged 10 commits into from Apr 12, 2019
Merged

Setup gradual process for migrating to Typescript #495

merged 10 commits into from Apr 12, 2019

Conversation

s1na
Copy link
Contributor

@s1na s1na commented Apr 11, 2019

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:

  • Dropped babel dependencies
  • Added a simple (not too strict) tsconfig.json which allows JS code. All code is transpiled via tsc (even JS)
  • Consequently JS code can't import TS files directly. Therefore I've changed the API tests to use the compiled version (dist) rather than source. This is until all source code has been migrated and can be later on reverted

For the first round, I've migrated Bloom, Stack and Memory (easier picks 馃槃):

  • Bloom's API has also been limited to only accept Buffer. Since this was only used internally, it shouldn't be a breaking change
  • Stack also only accepts BN (dropped Buffer as it wasn't being used)
  • Added rlp as a dev dependency (due to version mismatch in ethereumjs-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)

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 95.501% when pulling 9eb3b70 on typescript into 484a339 on v4.

Copy link
Member

@holgerd77 holgerd77 left a 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]
Copy link
Member

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK (file).

}

return false
}
}
Copy link
Member

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",
Copy link
Member

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()
})

Copy link
Member

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/**/*"]
}
Copy link
Member

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.

@holgerd77
Copy link
Member

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.

@s1na
Copy link
Contributor Author

s1na commented Apr 12, 2019

prettier/formatting is not working yet, or is it?

Well, the command runs, but I haven't fixed the errors :-D They were mostly on the md files.

Side note 2

Sure! Until Monday I'll be working very sporadically and can't really plan it, but from Monday we can divide and conquer together ;)

@s1na s1na merged commit affe8b4 into v4 Apr 12, 2019
@s1na s1na mentioned this pull request Apr 12, 2019
23 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants