Migrate to Typescript #96
Conversation
|
This pull request fixes 1 alert when merging b6247ff into 1c4c539 - view on LGTM.com fixed alerts:
|
|
This pull request fixes 1 alert when merging 9a9a033 into 5a5b1a1 - view on LGTM.com fixed alerts:
|
|
@s1na Have updated this branch with the Travis xvfb fix, now the main tests generally pass but there seems to be some babel version related problem left preventing the |
|
I haven't reviewed the code but got curious about the xvfb thing, so I tried running the tests locally and I'm getting a different set of errors. I think there's no need for babel here, so I copied the It's here: module.exports = function(config) {
config.set({
browserNoActivityTimeout: 60000,
frameworks: ['browserify', 'detectBrowsers', 'tap'],
files: ['./test/*.js'],
preprocessors: {
'./dist/**/*.js': ['browserify'],
'./test/**/*.js': ['browserify']
},
singleRun: true,
detectBrowsers: {
enabled: true,
usePhantomJS: false,
postDetection: function(availableBrowsers) {
return ['Firefox']
},
}
})
}This changes also let us remove the file |
|
This pull request fixes 1 alert when merging 1a2400a into 5a5b1a1 - view on LGTM.com fixed alerts:
|
|
This pull request fixes 1 alert when merging eebf74b into 5a5b1a1 - view on LGTM.com fixed alerts:
|
|
This pull request fixes 1 alert when merging e326642 into 5a5b1a1 - view on LGTM.com fixed alerts:
|
|
Ok, tests working, seems coverage run needs some additional fix? |
|
I think it might have something with the fact that we run tests on Another point is that when I run |
|
By the way, thanks @alcuadrado for the fixed karma config :) |
|
This pull request fixes 1 alert when merging 910cd20 into 5a5b1a1 - view on LGTM.com fixed alerts:
|
|
Modified |
|
(closing and reopening this PR, as that supposedly fixes coveralls) |
|
That didn't work. We should try this, but I don't have enough permissions. @holgerd77 maybe? |
|
This pull request fixes 1 alert when merging 467a457 into 5a5b1a1 - view on LGTM.com fixed alerts:
|
|
Finally! Either updating the Coverage went down 2%, probably because it's measuring coverage over the compiled files and not the sources. Not sure what to do about it Before this, I also added CircleCI config files (to test if |
|
Great! 😄 I would have a tendency to not side-introduce CI tool switches here and keep on Travis at least along with this PR. |
|
@s1na Have increased the failure decrease threshold, for the TS transitions I think it's ok to make an exception with all these heavy structural changes likely shifting things a bit. There is still a 90% absolute threshold which will cover us here. |
|
This pull request fixes 1 alert when merging c693c65 into 5a5b1a1 - view on LGTM.com fixed alerts:
|
|
Ok, removed the circleci config, but only afterwards removed this repo from the circleci app, that's why it tried to build but failed. If necessary I can do a rebase after the review to "fix" the circleci error. |
|
Yes, maybe a rebase would be good here, especially for these large/central PRs it looks a bit ugly if CI is displayed as "failing". Can you actually do before the review, otherwise the approval will be again dismissed on rebase? |
|
Update: or maybe wait, I'll likely have some small comments to update. |
|
My main concern for rebasing was that Github might not detect modifications to the files and doesn't show a good diff, which would make the review process harder. |
There was a problem hiding this comment.
Sometimes a review is already worth to be done to be able to fully appreciate other ones work, wasn't aware that the update on this library was so massive and extensive, thanks for giving this such a direct shot. 👍 😄
Some comments, some questions, some change requests, but generally looks pretty good and complete already.
src/baseTrie.ts
Outdated
| const ReadStream = require('./readStream') | ||
| const PrioritizedTaskExecutor = require('./prioritizedTaskExecutor') | ||
| const { callTogether } = require('./util/async') | ||
| const { stringToNibbles, matchingNibbleLength, doKeysMatch } = require('./util/nibbles') | ||
|
|
||
| /** | ||
| * Use `require('merkel-patricia-tree')` for the base interface. In Ethereum applications |
There was a problem hiding this comment.
Can you update this introductory comment text here as well with the accurate require instructions?
|
coveralls error: |
|
This pull request fixes 1 alert when merging a07c39f into 5a5b1a1 - view on LGTM.com fixed alerts:
|
|
@holgerd77 Thanks a lot for doing this extensive review. For several comments I had to double-check to see if I had made a mistake. Glad to know if I sometime do make a mistake, you'll likely catch it! A note on the new errors and asserts: In those places errors were possible, it's just that before the code would have failed a bit further with an unrelated error such as |
|
Ah, CI is not working, re-trigger didn't work. |
|
Hmm, coverage looks good on Travis. This seems to be some (hopefully) temporary problem with Coveralls. Have temporarily disabled the check as being required and will merge now. |
|
Yeah coveralls is down since yesterday (even their homepage). Hopefully it'll be fixed soon. Thanks for merging. |
|
@s1na Just read your comment about the TS release on the other issue thread. Would it be worth to do an in-between major release here? This is hanging around for quite some time and then we would have a new work basis, eventually this would also ease the transition for people if they can do this in-between hop. Or are you planning to integrate the refactoring changes from #100 and subsequent PRs on a very short-term basis? |
If we decide it's better to release all changes together I'd be okay with making the changes in a relatively short time frame.
Now whether it makes sense to do a major release before making additional changes: there have been a lot of changes that have not yet been released. Considering that doing a release is sensible. The good thing about doing a release now is that as you said the breaking changes are not that much and it'd be easier to migrate. On the other hand, in it's current form the codebase is not very polished. I had to have a look at the recent PRs and here I wrote So in short, the current state is not so bad not to be releasable, but there is a lot of room for improvement. What do you think @holgerd77? |
|
@s1na There not too much time pressure regarding a major release, apart from the fact that it would be nice that others can benefit of the improvements made. If you can do the simplifications/refactorings within the next 2-3 weeks I would be fine with waiting for a larger major release. We just shouldn't draw this release beyond the Christmas holidays. |
|
Okay, let's target end of November as a target for release. |
Breaking changes are how classes are exported (using named exports). Users can import one of
CheckpointTrie, SecureTrie, BaseTrievia `import { SecureTrie } from 'merkle-patricia-tree'.I don't consider this migration complete. The library in its current is difficult to type and I had to use a lot of type casting (e.g.
key as Buffer) and ignoring TS errors via// @ts-ignorefor now to get it to compile, and to avoid making this PR bigger than it is. Some parts really need refactoring to have proper types (speciallyTrieNode), which I plan to do in next PRs. Promisification is also left for future PRs.I've tested this branch in
ethereumjs-vmand tests are passing