Upgrade playground-io #12979

islemaster · 2017-01-31T19:33:51Z

Update playground-io from @bcjordan's fork back to mainline v0.4.0 (rwaldron/playground-io@0.4.0, diff of this upgrade). Pair: @bcjordan.

Major API change in the upgrade is the introduction of a Touchpad controller to help treat the capacitive touch sensors as buttons. We in turn wrap this component into individual objects for each touch sensor to create the requested

Also fixes an onBoardEvent dropdown bug: When the first parameter to onBoardEvent was not recognized, the code generating the second param dropdown raised an error because its lookup against the first param failed. Now when it's not recognized it will list all possible second parameter values. Includes tests over the new helper method.

Finally there's quite a bit of cleanup, and commenting, as I'm still getting to know the maker code.

Testing

Added unit tests for the TouchSensor wrapper and for the dropdown bug that got fixed.

Manually tested the eight cap-touch sensors, ten neopixels, and two buttons via App Lab+Maker Toolkit.

codecov-io · 2017-01-31T20:32:33Z

Codecov Report

❗ No coverage uploaded for pull request base (staging@095d55d).

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 095d55d...c82019b. Read the comment docs.

Bjvanminnen · 2017-02-01T03:03:13Z

apps/package.json

@@ -138,7 +138,7 @@
    "node-sass": "^4.0.0",
    "npm-which": "2.0.0",
    "phantomjs-prebuilt": "^2.1.3",
-    "playground-io": "bcjordan/playground-io#tap-cap-touch",
+    "playground-io": "0.4.0",


Bjvanminnen · 2017-02-01T03:05:38Z

apps/src/makerlab/PlaygroundComponents.js

@@ -74,7 +79,7 @@ export function initializeCircuitPlaygroundComponents(io, five, PlaygroundIO) {
    addSensorFeatures(five.Board.fmap, s);
  });

-  return _.assign({}, touchSensors, {
+  return Object.assign({}, touchPads, {


Could also do this using object destructuring (which has become my general preference)

return { ...touchPads, colorLeds: colorLeds, // etc }

I keep forgetting I can do this. That's way cleaner. Thanks!

Bjvanminnen · 2017-02-01T03:06:52Z

apps/src/makerlab/TouchSensor.js

+
+    /**
+     * @private {number} Which single pin this controller will monitor and
+     *          forward events for.


Do we always indent so much in comments?

I habitually indent past the @tag for line wraps in JSDoc comments (have been doing this for a while) because I think it reads nicely in parameter lists, but I don't know what our preferred style is. It looks like Use JSDoc doesn't illustrate any indenting. Do you have a preference?

I know I don't indent like that (i just indent the next line two spaces). I also don't think it's that big of a deal if our styles are slightly different in this respect.

Bjvanminnen · 2017-02-01T03:09:13Z

apps/test/unit/makerlab/TouchSensorTest.js

+const UP = 'up';
+
+describe('TouchSensor', function () {
+  let fakeTouchPad, touchSensor, spy;


not sure whether/if we call this out in our style doc, but i think we usually put each of these on separate lines.

Looks like you're right and this is called out by AirBnB rule 13.2, enforced via ESLint one-var.

We've never enforced this - ESLint finds 8017 violations of this rule in /apps right now, and unfortunately it's not autofixable.

Personally, I prefer the "one-var": ["error", {"initialized": "never"}], variant on this rule that enforces one-var-per-declaration for initialized variables, but permits lists of uninitialized variables like this one. For what it's worth, there are only 62 violations of that rule right now. It might be a good place to start if we want to begin enforcing this.

I don't feel strongly about this (tho I guess we should either have a lint rule, or I shouldn't call it out as I see it in code reviews).

Hamms · 2017-02-01T18:44:15Z

apps/package.json

@@ -138,7 +138,7 @@
    "node-sass": "^4.0.0",
    "npm-which": "2.0.0",
    "phantomjs-prebuilt": "^2.1.3",
-    "playground-io": "bcjordan/playground-io#tap-cap-touch",
+    "playground-io": "0.4.0",


Hamms · 2017-02-01T18:45:48Z

apps/src/makerlab/BoardController.js

@@ -164,6 +159,9 @@ export default class BoardController {
  }

  onBoardEvent(component, event, callback) {
+    // TODO: Add accelerometer events for "singletap" and "doubletap" that map to


nit: could you assign this TODO?

Hamms · 2017-02-01T18:52:43Z

apps/src/makerlab/TouchSensor.js

+      this.touchpadsController_.removeListener('up', this.upHandler);
+      this.upHandler = null;
+    }
+    this.removeAllListeners();


given that start immediately removes all listeners, is this necessary? Or just here for safety?

“All through my life I've had this strange unaccountable feeling that something was going on in the world, something big, even sinister, and no one would tell me what it was."

"No," said the old man, "that's just perfectly normal paranoia. Everyone in the Universe has that.”

Good call - the removeAllListeners call in start always gets called after this one, and that in turn is a paranoid redundant call with this one in BoardController::resetComponent. I got a little reset-happy.

islemaster · 2017-02-02T00:24:50Z

Update: Right now this upgrade is stopped cold because unit tests are breaking on the upgrade to rwaldron/playground-io@0.4.0 with the following error:

PhantomJS 2.1.1 (Linux 0.0.0) ERROR
  SyntaxError: Unexpected token '>'
  at /home/brad/Projects/cdo/code-dot-org/apps/test/unit-tests.js:346725

Finished in 1.526 secs / 0 secs

SUMMARY:
✔ 0 tests completed

This is visible in recent circle builds on this branch and on my local machine. In fact, I find the following minimal change repro's the problem:

# Run from [cdo]/apps directory

# Get a clean staging build
git checkout staging

# Change the playground-io version in package.json and update dependencies
sed -i '/"playground-io"/ s/bcjordan\/playground-io#tap-cap-touch/0.4.0/' package.json && yarn

# Do a clean build
rm -rf build && npm run build

# Run unit tests
grunt karma:unit

# Expected: Tests run, maybe a few failures related to upgrade.
# Actual: No tests run, error message above.

We're not getting a decent sourcemapped location for the error, and the problem seems to only occur when running with PhantomJS - tests run when you use Chrome (BROWSER=Chrome WATCH=1 grunt karma:unit). I'm somewhat stuck on how to further debug this. I'm currently trying each playground-io commit in the upgrade diff to try and understand exactly what change introduced the problem.

islemaster · 2017-02-02T00:33:40Z

Testing playground-io commits in the update:

Commit	Tested	Second round
84b20ac	✅	✅
1cf5a8d	✅	❌
325934d	✅	❌
f85c244	❌	❌
3608c5b	❌
v0.4.0 (tag)	❌ (should be same as 3608c5b)

This makes it look like rwaldron/playground-io@f85c244 is the culprit - extra-surprising because that's a tiny diff, just removing a supposedly-unused dependency on something called bufferpack.

Even stranger, putting that dependency back (by depending on branch islemaster/playground-io@restore-bufferpack-dependency) doesn't fix the problem.

Update: After reverting f85c244 didn't seem to fix anything, I'm doing a second round of testing (and wondering if I wasn't clearing things out sufficiently well before). My new command at each commit is:

git checkout -- yarn.lock && rm -rf ~/.yarn-cache/npm-playground-io-* && rm -rf node_modules/playground-io && yarn && rm -rf build && npm run build && grunt karma:unit

So today, with this (hopefully more complete) test it looks like 1cf5a8d is the problem commit - which is more promising as it contains actual changes, but tracking down the offending change is going to be harder because it's a big commit.

islemaster · 2017-02-02T18:47:34Z

New strategy, assuming 1cf5a8d is the problem commit:

I've created a branch on my fork, find-breakage, at 84b20ac and I'm going to slowly reintroduce the changes from 1cf5a8d to work out what's causing the break.

Commit	Change	Pass?
find-breakage@84b20ac	Initial working commit	✅
find-breakage@6a00ab7	Introduce `lodash.debounce` dependency	✅
find-breakage@c068893	Import `lodash.debounce`	✅
find-breakage@39cff74	Introduce one arrow function	❌

It looks like our culprit is ES6 in the playground-io module - specifically, arrow functions, hence the Unexpected token '>' error. PhantomJS has no support for arrow functions. This lines up with everything working when we run tests on Chrome (which supports arrow functions) and in the actual build.

I thought that our transpiler covered this for imported modules too - or that we'd at least get a more useful error message. I'll have to track down why this is not the case here.

Quick solutions that come to mind:

Fork playground-io and...
- ...manually remove all arrow functions.
- ...or add a transpiler step and npm publish to our own namespace.
Ask upstream if we can remove the ES6 code (probably slow)
Fix our own test build process to transpile imported modules?
Don't run unit tests on PhantomJS anymore.

Hamms · 2017-02-02T19:36:38Z

I like the idea of transpiling more things for our test build process; do we have any idea how big of a work item that is?

islemaster · 2017-02-02T19:51:34Z

Fairly minimal effort - right now we only transpile src/, test/ and our @cdo/ aliases. I'm throwing together a change that specifically adds playground-io to this list and gets tests working again.

I'm not thrilled with the idea of maintaining a list of packages that need transpilation, but neither do I think it's a good idea to always transpile all of our dependencies (which may be prohibitively slow). I do think I prefer the transpilation list to a fork of playground-io with a transpile pipeline though - at least that way we get to depend on the official version.

islemaster · 2017-02-02T20:06:28Z

Okay, proposed solution for now: 7a9118a

Feedback please!

Hamms · 2017-02-02T20:16:38Z

Woah, we don't even transpile other installed modules in production? That seems odd; I'd be interested to see how much slowdown we actually get from adding that step.

Otherwise, this approach seems like a great first step!

islemaster · 2017-02-02T20:39:18Z

Timing reported by npm run build across three trials for each scenario.

Current	+ `playground-io` only	+ all of `node_modules`
48.8s	49.7s	...
49.4s	49.6s	...
49.3s	48.8s	...

Trying to transpile all of node_modules runs for 134s for me before failing with an out-of-memory error 😆.

Update playground-io from bcjordan/playground-io#tap-cap-touch branch to official rwaldron/playground-io@0.4.0. This commit temporarily removes support for capacitive touch sensors because in addition to the API change the new version doesn't provide support for lazy-enabling sensors and hurts overall performance. Getting this change checked in with the one known regression and fixing up before merge. Includes updated yarn.lock

Wraps playground-io's Touchpad controller in the (re-introduced) TouchSensor controller which surfaces up/down events for a single pin.

When the first parameter to onBoardEvent was not recognized, the code generating the second param dropdown raised an error because its lookup against the first param failed. Now when it's not recognized it will list all possible second parameter values. Includes tests over the new helper method.

islemaster force-pushed the upgrade-playground-io branch from ffffd63 to 660497c Compare January 31, 2017 20:32

islemaster force-pushed the upgrade-playground-io branch from c28a33f to ae87ce0 Compare February 1, 2017 02:32

islemaster changed the title ~~[WIP] Upgrade playground-io~~ Upgrade playground-io Feb 1, 2017

islemaster requested review from Hamms and Bjvanminnen February 1, 2017 02:59

islemaster assigned bcjordan and islemaster Feb 1, 2017

Bjvanminnen approved these changes Feb 1, 2017

View reviewed changes

Hamms approved these changes Feb 1, 2017

View reviewed changes

islemaster force-pushed the upgrade-playground-io branch from 022911d to 0f05058 Compare February 1, 2017 23:02

islemaster added 11 commits February 2, 2017 12:52

Enable touchpad

8da4e6c

Implement new touchPadN boardEvent API

6c65f18

Wraps playground-io's Touchpad controller in the (re-introduced) TouchSensor controller which surfaces up/down events for a single pin.

Reset TouchSensor components on run/reset

9f1b8af

Clarifying comments [ci skip]

563ab42

Smaller diff in PlaygroundConstants

06ffcd8

Introduce TouchSensorTest

fe5a651

Test TouchSensor reset behavior

c32f2fc

Splat touchPads

94e92a0

Assign TODO [ci skip]

63c0dcb

islemaster added 2 commits February 2, 2017 12:52

Remove redundant removeAllListeners call

106606a

Transpile playground-io

5553af2

islemaster force-pushed the upgrade-playground-io branch from 7a9118a to 5553af2 Compare February 2, 2017 20:52

islemaster merged commit 0c49f4d into staging Feb 2, 2017

islemaster deleted the upgrade-playground-io branch February 2, 2017 21:46

davidsbailey mentioned this pull request Feb 14, 2017

Stop caching apps/.babel-cache in circle ci #13239

Merged

islemaster mentioned this pull request Mar 16, 2017

Maker: Fix accelerometer #13843

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade playground-io #12979

Upgrade playground-io #12979

islemaster commented Jan 31, 2017 •

edited

codecov-io commented Jan 31, 2017 •

edited

Bjvanminnen Feb 1, 2017

Hamms Feb 1, 2017

Bjvanminnen Feb 1, 2017

islemaster Feb 1, 2017

Bjvanminnen Feb 1, 2017

islemaster Feb 1, 2017

Bjvanminnen Feb 1, 2017

Bjvanminnen Feb 1, 2017

islemaster Feb 1, 2017

Bjvanminnen Feb 1, 2017

Hamms Feb 1, 2017

Hamms Feb 1, 2017

Hamms Feb 1, 2017

islemaster Feb 1, 2017

islemaster commented Feb 2, 2017

islemaster commented Feb 2, 2017 •

edited

islemaster commented Feb 2, 2017 •

edited

Hamms commented Feb 2, 2017

islemaster commented Feb 2, 2017

islemaster commented Feb 2, 2017

Hamms commented Feb 2, 2017

islemaster commented Feb 2, 2017 •

edited

Upgrade playground-io #12979

Upgrade playground-io #12979

Conversation

islemaster commented Jan 31, 2017 • edited

Testing

codecov-io commented Jan 31, 2017 • edited

Codecov Report

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

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

islemaster commented Feb 2, 2017

islemaster commented Feb 2, 2017 • edited

islemaster commented Feb 2, 2017 • edited

Hamms commented Feb 2, 2017

islemaster commented Feb 2, 2017

islemaster commented Feb 2, 2017

Hamms commented Feb 2, 2017

islemaster commented Feb 2, 2017 • edited

islemaster commented Jan 31, 2017 •

edited

codecov-io commented Jan 31, 2017 •

edited

islemaster commented Feb 2, 2017 •

edited

islemaster commented Feb 2, 2017 •

edited

islemaster commented Feb 2, 2017 •

edited