Skip to content
This repository has been archived by the owner on Jan 19, 2021. It is now read-only.

Add support for ES6 classes #57

Merged
merged 3 commits into from
Nov 9, 2018
Merged

Add support for ES6 classes #57

merged 3 commits into from
Nov 9, 2018

Conversation

alextsg
Copy link
Contributor

@alextsg alextsg commented Oct 30, 2018

No description provided.

@holgerd77
Copy link
Member

You can just throw Node 4,5 and 7 here out on the way for Travis build, also there is still some linting error.

@holgerd77
Copy link
Member

Greetings from Devcon, you are missed! Already hooked up with @whymarrh. 😄

@holgerd77
Copy link
Member

Are you updating baseTrie.js as well?

And just a reminder that the docs have to be recreated at the end of the process.

@coveralls
Copy link

coveralls commented Oct 30, 2018

Coverage Status

Coverage decreased (-0.1%) to 94.437% when pulling 1b421fd on i44-es6 into 192a06a on master.

@alextsg
Copy link
Contributor Author

alextsg commented Oct 30, 2018

Hey @holgerd77, thanks for the comments. Will look into the docs, but this PR was mostly just to allow for the use of ES6 classes and remove the dist folder from the repo, so I was planning to update baseTrie.js afterwards. Glad you were able to meet up with @whymarrh!

@holgerd77
Copy link
Member

This dist folder generally seems to be an extra thing to have some browser-compatible library version build. With this PR we probably also should reorganize to distribute a version transpiled with babel (maybe from src to the root directory and then exclude the files via .gitignore, not sure but I can remember that we had after such a change and then building to a directory problems like that requires like require('merkle-patricia-tree/secure') also had to change path, not sure?).

Otherwise the distribution build probably will not work properly with these static member changes?

@alextsg
Copy link
Contributor Author

alextsg commented Oct 31, 2018

@holgerd Thanks for the heads up. So from what I understand, dist currently contains trie.js, which is built from index.js and contains CheckpointTrie. It does not, however, contain secure.js or SecureTrie because that’s not required, directly or recursively, in index.js. The build step for dist/trie.js is also important because it sets Trie as a window global.

I believe the solution, in order to preserve the current behavior and allow for require('merkle-patricia-tree/secure’) would be to transpile index.js to dist/index.js, and also transpile secure.js to dist/secure.js, then set ”main”: “dist/index.js” in package.json. We can also create the src folder for better organization. Does this sound correct?

@holgerd77
Copy link
Member

Not quite, I think dist atm is a folder which has no concrete functionality and only has the function to provide some nice-to-have browser build, as far as I see is.

Current distribution is done on the Node compatible code and we probably want to keep it that way for the moment. What I was suggesting would be that we keep the dist build as-is, move the current files in root to a directory src and do a separate transpile to ES5-Node-compatible code to the root directory and then exclude these files in .gitignore.

I know this is not the super-elegant solution, but this works at least, I can now remember, we had this discussion in the wallet library here ethereumjs/ethereumjs-wallet#65 and there was not an easy obvious fix.

If you have got a better idea though I am also open to this, we just have to make sure things work at the end! 😄

@alextsg
Copy link
Contributor Author

alextsg commented Oct 31, 2018

Ah, ok I think I understand now. Thanks! Will make changes.

@holgerd77
Copy link
Member

Hi @alextsg, just merged the leveldb update PR from Vinay @vpulim #56, can you rebase your PR on top (of master)? Sometime after merge I would like to do a v3.0.0 release, do you plan to do a follow-up PR on this before?

@alextsg
Copy link
Contributor Author

alextsg commented Nov 6, 2018

Hi @holgerd77 , Yep, will update this PR soon. Thanks for the heads up.

@alextsg alextsg force-pushed the i44-es6 branch 2 times, most recently from 7ee3e53 to 7657dc9 Compare November 8, 2018 23:19
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.

This looks good, will merge for now, thanks! 😄

So if I get this right, there is one follow-up PR to be done, is BaseTrie.js the only file that still needs an update? When you do the next PR, can you also include rebuild documentation?

Cheers
Holger

index.js Outdated
checkpointInterface.call(this, this)
static verifyProof (...args) {
return proof.verifyProof(...args)
}
Copy link
Member

Choose a reason for hiding this comment

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

This look equivalent.

node_modules
dist/
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 make sense to throw out the dist directory, cleaner like this.

secure.js
trieNode.js
util.js
Copy link
Member

Choose a reason for hiding this comment

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

I admit this solution is not super-pretty, but since there are also other .js files in the directory it's probably necessary to be so explicit here. I also double checked again, there seems to be no non-workaround solution in Node to set the import base path for modules (everyone correct me if I am wrong).

"@babel/plugin-proposal-class-properties": "^7.1.0",
"@babel/preset-env": "^7.1.0",
"babel-tape-runner": "^3.0.0",
"babelify": "^10.0.0",
"browserify": "^13.0.0",
"coveralls": "^2.11.6",
"documentation": "^3.0.4",
Copy link
Member

Choose a reason for hiding this comment

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

Ok, build changes seems to be ok.

static verifyProof (...args) {
return proof.verifyProof(...args)
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Ok.

})
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Ok.

})
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Ok.

constructor (...args) {
super(...args)
secureInterface(this)
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Ok.

@@ -1,4 +1,4 @@
const Trie = require('../index.js')
const Trie = require('../src/index.js')
const describe = require('tape')

describe('kv stream test', function (tester) {
Copy link
Member

Choose a reason for hiding this comment

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

Test changes also ok.

@holgerd77 holgerd77 merged commit 3b96e3a into master Nov 9, 2018
@holgerd77 holgerd77 deleted the i44-es6 branch November 9, 2018 09:53
@holgerd77
Copy link
Member

One other note: just took some step back and did another look. I have no ideas actually about this whole benchmarking stuff, if this is still working and what's the status there. Maybe you can have a short look on this on next update and - if these are just two paths modifications - do it along (and maybe also add 1-2 lines about usage on the README) - or otherwise open a new issue on this. Thanks!

@holgerd77
Copy link
Member

Follow-up note (another one): I am still not completely happy with this distribution structure, it's working but not particularly elegant. Optimal would be: build everything to a distribution directory (then probably dist and current dist renamed to browser or something) with import paths staying unchanged.

If you have good ideas on this let me know.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants