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

Move to our new eslint standard, update to use let/const #104

Merged
merged 15 commits into from
Aug 10, 2017
Merged

Conversation

sgtcoolguy
Copy link
Contributor

This moves to using the new appcelerator/eslint-config-axway configuration for listing the code. This involved a lot of changes to adhere.

I also moved to using let/const in place of var, since the code originally was written in a way that sort of assumed block scoping for var (I had to hoist them all up to make jshint/jscs happy in the previous round of cleanup, but they were declared/sprinkled where used, not at top of scope before this).

I also updated to use node-minify via babili to minify, rather than uglifyjs (since it can't handle const/let).

Previous to this PR, I had moved from using a Makefile to using grunt and plugins to make this more like our other projects - and had moved to using appcJS to lint via jshint/jscs (which we're now moving away from to eslint)

@sgtcoolguy sgtcoolguy requested a review from feons August 7, 2017 14:33
@ewanharris
Copy link
Contributor

@sgtcoolguy I think we should look at adding some preset-env or something here, I'm not full confident in all of our supported target OS's/JS engines supporting the usage of let/const.

Additionally, so as we only allow one version of liveview at at time due to us shipping in Studio I presume we would need to keep this compatible with version of the SDK at least in our current major line (6_X), and maybe even before?

Excuse the drive by review if I'm missing something and this has already been discussed :)

require('shelljs/global');

var program = require('commander'),
colors = require('coloring'),
Copy link
Contributor

Choose a reason for hiding this comment

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

This is used, eg. line 80, [ERROR]'.red

* Module dependencies.
*/
var program = require('commander'),
colors = require('coloring'),
Copy link
Contributor

Choose a reason for hiding this comment

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

This is needed.

// inject shelljs to the global scope
/* globals exec */
require('shelljs/global');

var program = require('commander'),
colors = require('coloring'),
Copy link
Contributor

Choose a reason for hiding this comment

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

This is needed.


// inject shelljs to the global scope
/* globals exec */
require('shelljs/global');

var program = require('commander'),
colors = require('coloring'),
Copy link
Contributor

Choose a reason for hiding this comment

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

This is needed.

@feons
Copy link
Contributor

feons commented Aug 9, 2017

liveview swapped liveview.js with app.js, liveview.js is run by the SDK. Everything under lib/platform/* will be built into liveview.js. So no const/let in those files I suppose.

@sgtcoolguy
Copy link
Contributor Author

@ewanharris @feons No, you guys are both right. I wasn't aware that the liveview.js got injected into the user's app via the hook script. That means we can't use const/let in that file.

To fix that, I dumped the minify step in our grunt file because we don't ever actually use liveview.min.js - and to instead first concat to build/liveview.es6.js and then use babel's es2015 preset to transpile to an ES5.1 compatible build/liveview.js. That should work on older SDKs.

It also means we could use more es6 features in the original code (to improve maintenance/readability) and it'll all get transpiled down for us - and that eventually we could avoid transpilation or bump the min target preset used as our SDKs can handle more.

@sgtcoolguy
Copy link
Contributor Author

I took out node 4 testing on travis because we need to set 'use strict' in the code to use const/let on Node 4.x. I did so on the bin scripts, but didn't want to put it in lib/fserver.js since that get's concatenated in to produce the final ES-compatible liveview.js and I didn't want that statement in the middle of the file.

fsWatcher = require('chokidar'),
colors = require('coloring'),
Copy link
Contributor

Choose a reason for hiding this comment

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

This is used, eg. line 51.

@feons
Copy link
Contributor

feons commented Aug 10, 2017

@sgtcoolguy,

@feons feons closed this Aug 10, 2017
@feons feons reopened this Aug 10, 2017
@feons
Copy link
Contributor

feons commented Aug 10, 2017

@sgtcoolguy, lib/fserver.js doesn't get concatenated in to produce liveview.js, only the files under lib/platform/ are used.

* @return {Array} running liveview server pid filenames
*/
FServer.pids = function (env) {
var pids = [];
let pids = [];
Copy link
Contributor

Choose a reason for hiding this comment

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

probably need to set use strict.

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

Successfully merging this pull request may close these issues.

None yet

3 participants