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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/hackathon may 18 migrate to es6 and make npm #1

Open
wants to merge 17 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 15 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions .babelrc
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
{
"presets": ["es2015", "react"],
"plugins": ["transform-es2015-modules-commonjs","dynamic-import-webpack"]
}
3 changes: 3 additions & 0 deletions .eslintignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
/coverage/*
/versions/*
/dist/*
28 changes: 28 additions & 0 deletions .eslintrc.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
{
"env": {
"browser": true,
"node": false
},
"parser": "babel-eslint",
"extends": ["eslint:recommended", "standard"],
"rules": {
"no-console": 0,
Copy link
Author

Choose a reason for hiding this comment

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

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.

"no-useless-call": 0,
"no-unused-vars": ["error", { "args": "after-used" }],
"indent": ["error", 4, { "SwitchCase": 1 }],
"semi": ["error", "always"],
"padded-blocks": 0,
"valid-jsdoc": ["error", {
"requireParamDescription": false,
"requireReturnDescription": false
}],
"require-jsdoc": ["error", {
"require": {
"FunctionDeclaration": true,
"MethodDefinition": true,
"ClassDeclaration": true,
"ArrowFunctionExpression": false
}
}]
}
}
46 changes: 0 additions & 46 deletions .vscode/launch.json

This file was deleted.

89 changes: 58 additions & 31 deletions ApprovedOrigins.js
Original file line number Diff line number Diff line change
@@ -1,40 +1,67 @@
var APPROVED_ORIGINS_KEY = "wdc_approved_origins";
var SEPARATOR = ",";
var Cookies = require('cookies-js');
import * as Cookies from 'cookies-js';

function _getApprovedOriginsValue() {
var result = Cookies.get(APPROVED_ORIGINS_KEY);
return result;
}
const SEPARATOR = ',';
export const APPROVED_ORIGINS_KEY = 'wdc_approved_origins';
// alias of Cookies for testing ( and for future replacement? )
export let CookiesLib = Cookies;

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

Choose a reason for hiding this comment

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

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);.

Copy link
Author

Choose a reason for hiding this comment

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

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


function _saveApprovedOrigins(originArray) {
var newOriginString = originArray.join(SEPARATOR);
console.log("Saving approved origins '" + newOriginString + "'");

// We could potentially make this a longer term cookie instead of just for the current session
var result = Cookies.set(APPROVED_ORIGINS_KEY, newOriginString);
return result;
return result;
}

// Adds an approved origins to the list already saved in a session cookie
function addApprovedOrigin(origin) {
if (origin) {
var origins = getApprovedOrigins();
origins.push(origin);
_saveApprovedOrigins(origins);
}
/**
*
* @param {Array<String>} originArray
* @returns {*}
*/
function _saveApprovedOrigins (originArray) {

const APPROVED_ORIGINS = originArray.join(SEPARATOR);

console.log(`Saving approved origins ${APPROVED_ORIGINS} `);

// We could potentially make this a longer term cookie instead of just for the current session
let result = CookiesLib.set(APPROVED_ORIGINS_KEY, APPROVED_ORIGINS);

return result;
}

// Retrieves the origins which have already been approved by the user
function getApprovedOrigins() {
var originsString = _getApprovedOriginsValue();
if (!originsString || 0 === originsString.length) {
return [];
}
/**
* Adds an approved origins to the list already saved in a session cookie
Copy link

Choose a reason for hiding this comment

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

I think origins should be singular

Copy link
Author

Choose a reason for hiding this comment

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

good catch

Copy link
Author

Choose a reason for hiding this comment

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

done

* @param {String} origin
* @returns {Object}
*/
export function addApprovedOrigin (origin) {

if (origin) {
let origins = getApprovedOrigins();

origins.push(origin);

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

Copy link

Choose a reason for hiding this comment

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

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

Copy link
Author

@Jaxolotl Jaxolotl May 16, 2018

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

done

var origins = originsString.split(SEPARATOR);
return origins;
}

module.exports.addApprovedOrigin = addApprovedOrigin;
module.exports.getApprovedOrigins = getApprovedOrigins;
/**
* Retrieves the origins which have already been approved by the user
* @returns {Array<String>}
*/
export function getApprovedOrigins () {

let originsString = _getApprovedOriginsValue();

if (!originsString || originsString.length === 0) {
Copy link

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

return [];
}

let origins = originsString.split(SEPARATOR);

return origins;
}
50 changes: 50 additions & 0 deletions ApprovedOrigins.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
/* eslint-env node, mocha, jest */
import * as ApprovedOrigins from './ApprovedOrigins';

// use if required for debugging
let consoleLog = console.log; // eslint-disable-line no-unused-vars
console.log = jest.fn();

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

beforeEach(() => {
// Cookies Mock to make this a Unit Test instead of an integration test
// temp mock, will investigate on a better mocking for Cookies lib :(
// (tried with standard jest mock, ridiculously failing)

ApprovedOrigins.CookiesLib.mokedCookies = {};

ApprovedOrigins.CookiesLib.get = jest.fn(() => {
return ApprovedOrigins.CookiesLib.mokedCookies[ApprovedOrigins.APPROVED_ORIGINS_KEY];
});

ApprovedOrigins.CookiesLib.set = jest.fn((key, val) => {
ApprovedOrigins.CookiesLib.mokedCookies[key] = val;

return ApprovedOrigins.CookiesLib;
});

});

it('getApprovedOrigins should get an empty array if no value is set', () => {
expect(ApprovedOrigins.getApprovedOrigins()).toEqual([]);
});

it('addApprovedOrigin should add an approved origin and ApprovedOrigins.getApprovedOrigins get it correctly', () => {
let origin = 'http://tableau.com';
expect(ApprovedOrigins.addApprovedOrigin(origin)).toBe(ApprovedOrigins.CookiesLib);

expect(ApprovedOrigins.getApprovedOrigins()).toEqual([origin]);

});

it('addApprovedOrigin should add multiple values and get an array of the values', () => {
let origin1 = 'http://tableau.com';
let origin2 = 'http://connectors.tableab.com';
ApprovedOrigins.addApprovedOrigin(origin1);
ApprovedOrigins.addApprovedOrigin(origin2);

expect(ApprovedOrigins.getApprovedOrigins()).toEqual([origin1, origin2]);

});
});
111 changes: 68 additions & 43 deletions DevUtils/BuildNumber.js
Original file line number Diff line number Diff line change
@@ -1,50 +1,75 @@
var fs = require('fs');
var path = require('path');
const execSync = require('child_process').execSync;

var WDC_LIB_PREFIX = "tableauwdc-";

function VersionNumber(versionString) {
var components = versionString.split(".");
if (components.length < 3) {
console.log()
throw "Invalid number of components. versionString was '" + versionString + "'";
}

this.major = parseInt(components[0]).toString();
this.minor = parseInt(components[1]).toString();
this.fix = parseInt(components[2]).toString();
}
import { version as BUILD_NUMBER } from '../package.json';
export let WDC_LIB_PREFIX = 'tableauwdc-';

VersionNumber.prototype.toString = function() {
return this.major + "." + this.minor + "." + this.fix;
}
/**
*
*/
export class VersionNumber {
/**
*
* @param {String} versionString
*/
constructor (versionString) {
let components = versionString.split('.');

VersionNumber.prototype.compare = function(other) {
var majorDiff = this.major - other.major;
var minorDiff = this.minor - other.minor;
var fixDiff = this.fix - other.fix;
if (components.length < 3) {
console.log();
throw new Error(`Invalid number of components. versionString was ${versionString} `);
}

if (majorDiff != 0) return majorDiff;
if (minorDiff != 0) return minorDiff;
if (fixDiff != 0) return fixDiff;
this.major = parseInt(components[0]).toString();
this.minor = parseInt(components[1]).toString();
this.patch = parseInt(components[2]).toString();
}

return 0;
}
/**
* @returns {String}
*/
toString () {
return `${this.major}.${this.minor}.${this.patch}`;
}

/**
*
* @param {Object} other
* @returns {Number}
*/
compare (other) {
let majorDiff = this.major - other.major;
let minorDiff = this.minor - other.minor;
let fixDiff = this.patch - other.fix;
Copy link

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

changed


if (majorDiff !== 0) {
return majorDiff;
}

if (minorDiff !== 0) {
return minorDiff;
}

function getBuildNumber() {
// Grab the version number from the environment variable
var versionNumber = process.env.npm_package_config_versionNumber;
if (versionNumber) {
console.log("Found versionNumber in environment variable: '" + versionNumber + "'");
} else {
versionNumber = process.argv.versionNumber;
console.log("Found versionNumber in argument: '" + versionNumber + "'");
}

return versionNumber;
if (fixDiff !== 0) {
return fixDiff;
}

return 0;
}
}

module.exports.VersionNumber = VersionNumber;
module.exports.WDC_LIB_PREFIX = WDC_LIB_PREFIX;
module.exports.getBuildNumber = getBuildNumber;
/**
*
* @param {Object} config
* @returns {String}
*/
export function getBuildNumber ({ showLog = false } = {}) {
// Grab the version number from the package json property ( one static source of truth )
Copy link

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

done


if (!BUILD_NUMBER) {
throw new Error(`Unable to retrieve version number from package.json`);
}

if (showLog) {
console.log(`Found versionNumber in package.json: ${BUILD_NUMBER}`);
}

return BUILD_NUMBER;
}
Loading