Rework export #13

boguslavsky · 2017-05-24T11:05:09Z

Implemented a flow described at issue #5.

slowli

Review as of 9dd3615. Good job overall, but I have 2 main issues:

It seems that the package won't work on Node without using require('babel-register') beforehand (which is what happens during testing). Transpiling code with Babel to a separate dir (say, lib) may be a better choice. This can be solved in a separate PR, though
this binding to manage imports is awkward (e.g., how would it work with non-function exports?); use import from individual files instead. I'd prefer for this to be solved here

slowli · 2017-05-24T11:22:13Z

src/blockchain/block.js


 /**
 * Verifies block
 * @param {Object} data
 * @param {Array} validators
 * @return {Boolean}
 */
-Exonum.verifyBlock = function(data, validators) {
-    if (Exonum.isObject(data) === false) {
+exports.verifyBlock = function(data, validators) {


If you use ES6's import statement and export * from ..., you may use

export function verifyBlock (data, validators) { // ... }

slowli · 2017-05-24T11:29:14Z

src/blockchain/block.js

-Exonum.verifyBlock = function(data, validators) {
-    if (Exonum.isObject(data) === false) {
+exports.verifyBlock = function(data, validators) {
+    var Block = this.newType({


I'm not sure I like binding to this everywhere. IMO, exports like this could be cleaner:

import * as types from './types'; // later const Block = types.newType({ // ... });

That is, you import exactly what you need from files you need.

slowli · 2017-05-24T11:32:10Z

src/blockchain/index.js

@@ -0,0 +1,5 @@
+'use strict';
+
+export * from './merkle';


ES6 exports 👍

slowli · 2017-05-24T11:59:12Z

Gruntfile.js

            }
        },
        mochaTest: {
            options: {
-                reporter: 'spec'
+                reporter: 'spec',
+                require: ['babel-register']


I'm not quite sure about this. I mean, if we want to publish to npm/yarn, we would want to transpile the source code using Babel to something that Node understands; afaik, Node does not understand import/export. Maybe, perform transpiling with the prepublish hook; also see this random repo that seems to do this. As a random (?) coincidence, note that we have source files in the src/ directory, which matches the scenario in which they are transpiled to the lib/ directory before the publication.

Putting it all together: it could be beneficial to have a build (or prepublish) Grunt task, which would transpile source files from src/ to lib/, and which could be plugged into scripts.prepublish section of package.json and other Grunt tasks (e.g., tests).

done, added prepublish scenario which prepare minified browser version (dist folder) and node version (lib folder).

slowli · 2017-05-24T12:03:28Z

src/blockchain/merkle-patricia.js

@@ -51,23 +48,23 @@ Exonum.merklePatriciaProof = function(rootHash, proofNode, key, type) {
        var elementsHash;

        if (typeof data === 'string') {
-            if (Exonum.validateHexHash(data) === true) {
+            if (self.validateHexHash.call(self, data) === true) {


As per a comment above, .call(self, ...) looks overly verbose. Wouldn't it be better to have something like

import { validateHexHash } from './validators'; // ... if (validateHexHash(data)) // ...

Additionally, in this case the function could be moved to the top level, improving readability.

boguslavsky · 2017-05-25T09:49:34Z

@slowli ready for review since c6ce6b1

P.S.: please do not merge #13 before #12 will be merged and all conflicts will be resolved

slowli

Mostly good as of 39e8477, with a couple of nitpicks.

slowli · 2017-05-25T10:04:34Z

Gruntfile.js

+        },
+        babel: {
+            options: {
+                sourceMap: true,


Are you sure source maps are needed here? The transpiled files are still human-readable, so they may be a little superfluous, imo. Maps add quite an overhead (~100% of the source size, on the first glance).

slowli · 2017-05-25T10:05:38Z

Gruntfile.js

-            dist: {
-                src: ['./dist']
-            }
-        },
        jshint: {
            files: ['./src/index.js'],


Nitpick: you should probably specify ./src/**/*.js here; otherwise, only a single file is linted.

moved to separate issue #15

slowli · 2017-05-25T10:26:04Z

src/index.js

@@ -1,10 +1,13 @@
-'use strict';
-var Exonum = require('../src/core');
+import * as types from './types/index';


This file could be simplified to

export * from './types'; export * from './crypto'; export * from './blockchain';

(Note that mentioning /index files is optional, as is the .js extension.) I don't quite like that every export is collected in a single namespace, but changing it would be a breaking change.

slowli · 2017-05-25T10:28:17Z

src/types/generic.js

+ * @param {Object} type
+ * @returns {NewType}
+ */
+export function newType(type) {


Don't quite like this code and the similar code for messages, but I suppose it's OK for the purpose of this PR.

slowli · 2017-05-25T10:35:14Z

.babelrc

@@ -0,0 +1,5 @@
+{


I'm wondering if .babelrc is actually needed if the Babel config is specified in the Grunt task. grunt prepublish seems to be working without this file.

slowli

LGTM as of 763b23a.

# Conflicts: # .gitignore # Gruntfile.js # dist/exonum-client.js # dist/exonum-client.min.js # src/blockchain/block.js # src/blockchain/merkle-patricia.js # src/blockchain/merkle.js # src/crypto/index.js # src/data-management.js # src/helpers.js # src/types/convert.js # src/types/primitive.js # src/types/serialization.js # src/types/validate.js

Ivan Bohuslavskyi added 2 commits May 24, 2017 11:32

create basic structure

f5fe00c

refactor existed code base according new code structure

9dd3615

boguslavsky mentioned this pull request May 24, 2017

Tracking issue for 0.1 release #14

Closed

9 tasks

slowli suggested changes May 24, 2017

View reviewed changes

Ivan Bohuslavskyi added 5 commits May 24, 2017 16:31

updated import syntax to ES6

ca56f3b

started rework bindings to dependencies

b2e525b

reworked bindings to dependencies

db096ef

remove 'use strict' flag

38360d5

fixed build of node package

c6ce6b1

remove minified files from dist

39e8477

slowli reviewed May 25, 2017

View reviewed changes

Ivan Bohuslavskyi added 4 commits May 25, 2017 14:03

fix babel scenario

27799f7

simplified exports

354f08e

fixed module entry point in tests

5c8338d

simplified export syntax

763b23a

slowli approved these changes May 25, 2017

View reviewed changes

Ivan Bohuslavskyi added 2 commits May 30, 2017 17:54

boguslavsky merged commit 826821f into exonum:master May 30, 2017

boguslavsky deleted the rework-export branch May 30, 2017 15:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework export #13

Rework export #13

boguslavsky commented May 24, 2017

slowli left a comment

slowli May 24, 2017

slowli May 24, 2017

slowli May 24, 2017

slowli May 24, 2017

boguslavsky May 25, 2017

slowli May 24, 2017

boguslavsky commented May 25, 2017 •

edited

slowli left a comment

slowli May 25, 2017

slowli May 25, 2017

boguslavsky May 25, 2017

slowli May 25, 2017 •

edited

slowli May 25, 2017

slowli May 25, 2017

slowli left a comment

Rework export #13

Rework export #13

Conversation

boguslavsky commented May 24, 2017

slowli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boguslavsky commented May 25, 2017 • edited

slowli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slowli May 25, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slowli left a comment

Choose a reason for hiding this comment

boguslavsky commented May 25, 2017 •

edited

slowli May 25, 2017 •

edited