Feature/hackathon may 18 migrate to es6 and make npm by Jaxolotl · Pull Request #1 · Tabloids/wdclib

Jaxolotl · 2018-05-15T14:10:44Z

@benlower and @LGraber : This pull request is merely informative, any feedback is hugely welcome. The merging target is sill OUR FORK, we need to talk and discuss before considering it for targeting the official repo.

Notes:

Since we don’t have Samm and Brendan maintaining the shim, and is a critical piece of code for our wdc’s I took the initiative to fork it and make this changes as a POC to match our development standards, this way might gain more collaborators.

Migrate the code to ES6
Make it an NPM private package to be consumed by our new connectors and built together with the connector’s code
Keep backwards compat to be able to generate the builds the old style and publish them for public consumption ( the npm script calls remain the same )
Add Linting validation as a build contract (must pass to build )
Add unit tests ( added some, more to come)
Add integration tests (TBD)
Deploy and publish using Travis CI (TBD)
Add JsDoc to all documentable methods and props ( need some refinement here yet )

I'd be great to define more specific tests:

Automated tests using our automation framework and consuming generic tests
Version targeted test ( testing shim against specific tableau versions based on support rules )
Testing browser compatibility for all versions
Create integration tests to match the Bridge behavior
Improve shim test-ability ( several methods just pass the ball to the bridge, or to another layer, but they don't communicate the success/failure status, and they don't even return a value )
Improve predict-ability ( some async methods just call setTimeout but there's no way to programatically verify the fulfillment or rejection. Implementing promises there, will significantly improve the usability and the error handling )

Tested this with local Anaplan connector as a standalone lib and with local QBO as part of the connector’s build, both in the simulator and Tableau desktop 10.5

- Convert ES5 constructor funtion to ES6 Classes - Added JsDoc - Added JsDoc rules to eslint - Retrieve BUILD_NUMBER from package.json ( one source of truth) ( bundle tested successfully with QBO v3 on simulator )

…g package.json as source of truth - Remove DefinePligin from webpack.config, we need the build number to be available when building from connectors - Importing i18n json instead of using json-loader ( avoiding conflicts when consumed as npm package from other building strategies ) - Smoke test pass as local npm package with QBO v3 and simulator

- improve messageHandlers.wdcHandler verification, and log a message if absent - Avoid double casting in condition

- disable no-unused-vars eslint rule on 2 lines for legacy documentation purpose

…ore build and converting to es5 before uglification - convert webpack-production script to ES6 - increments minor version (temporarily, will ask for that later)

- update package.json to run make_version script with babel cli, maintaining the ´previous ux - fix missing separator on VersionNumber.toString method body

- include babel-polyfill ( let's discuss about file size, IMHO the min version has a decent size and by including it we won't care anymore about accidental introduction of unsupported features on IE and Edge ... until we deprecate support for them ) - add information "engines" property to package json

…oid polluting the test output - add NativeDispatcher unit tests

Jaxolotl · 2018-05-15T14:12:21Z

+    "parser": "babel-eslint",
+    "extends": ["eslint:recommended", "standard"],
+    "rules": {
+        "no-console": 0,


unfortunately we have to leave the no-console rule set off globally because it's part of the application flow.
I'd like to improve this later.

Jaxolotl · 2018-05-15T14:14:52Z

Later I'll go through all the jsdocs and validate the @params and @returns tags against the API reference and also adding the @see tag with the link to the method documentation

whoan

Code looks great!
I wrote some comments to discuss further.

whoan · 2018-05-15T18:28:26Z

+ * @returns {*}
+ */
+function _getApprovedOriginsValue () {
+    let result = CookiesLib.get(APPROVED_ORIGINS_KEY);


Is there any reason to have an assignation here? If CookiesLib.get returns a primitive type, we could avoid a copy just returning: return CookiesLib.get(APPROVED_ORIGINS_KEY);.

Yup, good point. The 2 reasons are:

I tried to keep things as less modified as possible

Probably the original code's intent was to have a potential breakpoint for debugging

whoan · 2018-05-15T19:03:07Z

-    return [];
-  }
+/**
+ * Adds an approved origins to the list already saved in a session cookie


I think origins should be singular

whoan · 2018-05-15T19:08:47Z

+        // pass along the output of the private function
+        return _saveApprovedOrigins(origins);
+    }



It might be better to return something to comply with the JSDoc (or change the JSDoc which I know is a pending task)

you're right, I'll change it to {Object|Undefined}

whoan · 2018-05-15T19:19:15Z

+
+    let originsString = _getApprovedOriginsValue();
+
+    if (!originsString || originsString.length === 0) {


It seems like originsString.length === 0 is redundant here

yup, same thing here, just trying to keep as much as possible untouched

whoan · 2018-05-15T19:24:51Z

+    compare (other) {
+        let majorDiff = this.major - other.major;
+        let minorDiff = this.minor - other.minor;
+        let fixDiff = this.patch - other.fix;


Do we have a copy paste error here? patch vs fix.

Nope, patch is the correct definition for the third number on the semver definition, I left the fix word for the input to ask Ben about that, I'd go for both to be patch for consistency

whoan · 2018-05-15T19:36:22Z

+ * @returns {String}
+ */
+export function getBuildNumber ({ showLog = false } = {}) {
+    // Grab the version number from the package json property ( one static source of truth )


Does it miss some code here? It is not clear how the version number is retrieved.

you're right, it's explaining the BUILD_NUMBER constant origin, but it's not clear, I'll improve it

whoan · 2018-05-15T20:02:25Z

+    _triggerDataGathering (tablesAndIncrementValues) {
+
+        if (tablesAndIncrementValues.length !== 1) {
+            throw new Error(`Unexpected number of tables specified. Expected 1, actual  ${tablesAndIncrementValues.length.toString()}`);


In ${tablesAndIncrementValues.length.toString()}, the .toString() is not necessary anymore

whoan · 2018-05-15T20:04:05Z

+            throw new Error(`Unexpected number of tables specified. Expected 1, actual  ${tablesAndIncrementValues.length.toString()}`);
+        }
+
+        let tableAndIncremntValue = tablesAndIncrementValues[0];


There is a small typo in the variable name: tableAndIncremntValue (misses an e)

whoan

Great job, the code is very clear.
I added some comments about minor things like typos in comments and alike.

whoan · 2018-05-16T18:06:18Z

-    if (typeof src[key] === 'function') {
-      dest[key] = src[key];
+/**
+ * WARNIG - copies are BY REFERENCE


Small typo here

whoan · 2018-05-16T18:07:01Z

+        // note: add boolean return for testing purpose
+
+        // Do some quick validation that this data is the format we expect
+        // is this validation enought? shouldn't we throw an error? (Jax)


Small typo in this comment

ohhh enought -> enough

whoan · 2018-05-16T18:07:16Z

+
+        if (!Array.isArray(data)) {
+            // Log a warning because the data is not an array like we expected
+            // is this validation enought? shouldn't we throw an error? (Jax)


Copied typo here

whoan · 2018-05-16T18:09:29Z

+
+describe('UNIT - Utilities', () => {
+
+    it('copyFunctions to copy funtions from source to target', () => {


Small typo in funtions

whoan · 2018-05-16T18:11:53Z

+import fs from 'fs-extra';
+import path from 'path';
+
+let execSync = require('child_process').execSync;


Wasn't it possible to import here?

I didn't even try, will do

whoan · 2018-05-16T18:12:43Z

+    let versionNumber = new VersionNumber(getBuildNumber());
+
+    // Build up the name for the regular and minified versions of the file
+    let fullVersionName = `${WDC_LIB_PREFIX}${versionNumber.toString()}`;


I think toString() wouldn't be necessary here

is necessary because it's calling the toString() met6hod of the class explicitly .... you never know :P

whoan · 2018-05-16T18:45:28Z

+import SimulatorDispatcher from './SimulatorDispatcher';
+import { getBuildNumber } from './DevUtils/BuildNumber';
+
+let qwebchannel = require('qwebchannel');


Can't it go an import here?

whoan · 2018-05-16T18:52:56Z

+    if (runningOnTableau()) {
+        // Running on Tableau
+        // We have the tableau object defined
+        console.log(`Initializing NativeDispatcher, Reporting version number`);


Just FYI, I've seen it's better to not use String templates when not needed, but I think it's a matter of tastes though.

whoan · 2018-05-16T18:56:16Z

@@ -0,0 +1,12 @@
+import webpack from 'webpack';


I am pretty sure we can do tree shaking here.

@see

- add @see links to online documentation for SImulator and Native dispatchers - add some tweaks to linting rules

Jaxolotl added 15 commits May 8, 2018 11:33

Working partial migration to es6 - bundle working and tested with QBO v3

4ab801a

- Continueing migration to ES6

c9f1a59

- Convert ES5 constructor funtion to ES6 Classes - Added JsDoc - Added JsDoc rules to eslint - Retrieve BUILD_NUMBER from package.json ( one source of truth) ( bundle tested successfully with QBO v3 on simulator )

- Import Cookies module instead of require it

7a3083d

- improve messageHandlers.wdcHandler verification, and log a message if absent - Avoid double casting in condition

- define default values for Table constructor

3455d78

- disable no-unused-vars eslint rule on 2 lines for legacy documentation purpose

- add babelloader and eslint loader to webpack config for linting bef…

7f85858

…ore build and converting to es5 before uglification - convert webpack-production script to ES6 - increments minor version (temporarily, will ask for that later)

- migrate make_version to ES6 (up and running)

fff7691

- update package.json to run make_version script with babel cli, maintaining the ´previous ux - fix missing separator on VersionNumber.toString method body

- add Enums, Table and Utilities tests ( working on adding more )

1c1997b

- Add tests for ApprovedOrigins

175488e

- mock console.log and console.warn on test to verify calls and to av…

87f96e6

…oid polluting the test output - add NativeDispatcher unit tests

preparing v2.4.0-beta.1 for private package publication

528c4ef

remove private from package.json to be able to publish

ee66e11

- typo fix forNativeDispatcher public interface

20f246b

- add linting npm scripts

bb876e7

Jaxolotl requested review from LGraber, adelstrinidad, benlower, gestevez, guicejas, rodrigoBerlochi and whoan May 15, 2018 14:10

Jaxolotl commented May 15, 2018

View reviewed changes

Jaxolotl requested a review from sblasetti May 15, 2018 14:17

whoan reviewed May 15, 2018

View reviewed changes

whoan reviewed May 16, 2018

View reviewed changes

- incorporate code review corrections

35e9f21

- add @see links to online documentation for SImulator and Native dispatchers - add some tweaks to linting rules

guicejas removed request for gestevez and guicejas September 25, 2018 19:30

export ENUMS_DICTIONARY to be consumed outside

38307c5


		let originsString = _getApprovedOriginsValue();

		if (!originsString \|\| originsString.length === 0) {


		describe('UNIT - Utilities', () => {

		it('copyFunctions to copy funtions from source to target', () => {

Uh oh!

Conversation

Jaxolotl commented May 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Notes:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jaxolotl commented May 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

whoan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jaxolotl May 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

whoan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Jaxolotl commented May 15, 2018 •

edited

Loading

Jaxolotl commented May 15, 2018 •

edited

Loading

Jaxolotl May 16, 2018 •

edited

Loading