Pull Maker APIs dropletConfig portion out of App Lab. Use object dropdowns for pixel/led blocks. #8563
Conversation
- contains -> includes - update test to remove usage of deprecated boolean option (use explicit object instead)
- Pull out blocks and categories into makerlab/dropletConfig.js, conditionally merge in to dropletConfig - Add mergeConcatArrays util that concats arrays rather than overwriting.
|
Looks like a bunch of circle-ci failures |
|
|
||
| const pixelType = '[Pixel]'; | ||
| const colorPixelVariables = ['pixels', 'pixel0', 'pixel1', 'pixel2', 'pixel3', 'pixel4', 'pixel5', 'pixel6', 'pixel7', 'pixel8', 'pixel9']; | ||
| const simpleLedVariables = ['led'].concat(colorPixelVariables); |
There was a problem hiding this comment.
this could also be const simpleLedVariables = ['led', ...colorPixelVariables];
|
@Bjvanminnen updated based on feedback and to use immutable for deep merge with array concat. & added some more explicit tests for that merge & concat behavior. PTAL! |
| } | ||
|
|
||
| var baseImmutable = Immutable.fromJS(baseObject); | ||
| return baseImmutable.mergeWith(deepConcatMerger, overrides).toJS(); |
There was a problem hiding this comment.
This feels weird, coercing a JS object to be immutable just so that we can use the mergeWith functionality of Immutable.js, and then converting back to JS. Using lodash's mergeWith seems more natural to me, given that we're not really using Immutable for it's primary purpose at all here.
There was a problem hiding this comment.
To get _.mergeWith we'll need lodash > 4.x. Giving that a shot, got incompatible version warnings for current versions of grunt-config-watch, grunt-config-uglify, and a dependency of grunt-cli.
Upgrading lodash and upgrading those 3 packages leaves 39 unit test fails. Would likely want to use something like https://www.npmjs.com/package/lodash-migrate to scan for potential API changes / deprecations in our code base, though since not everything is unit test exercised it will probably warrant a more careful look at existing usages & UI test run.
Since this PR is a live site issue and hoping to get this out quickly, & we don't yet have another use case for a full lodash 4.x upgrade, and I'm really doing this just to extend one object and one array, I might lean towards (1) just doing this object merge & extend manually for this case, (2) using immutable as in current PR, or (3) introducing something standalone like deepmerge, though it has issues with mutability of nested properties, is looking for a new owner & isn't super widely used.
Leaning towards options 1 or 2, any preference?
There was a problem hiding this comment.
Slight preference for 1. Okay with 2 since it's probably fastest.
There was a problem hiding this comment.
Added TODO note to swap out [Immutable].mergeWith with _.mergeWith once lodash upgraded to 4.x per JS pipeline work item. Slightly prefer having the mutability behavior of this util specified, and will be an easy swap-in as lodash becomes available (confirmed _.mergeWith technique passes same unit tests).
I noticed that blocks like
led.on, while not included in the default App Lab template list of blocks, were still being included in autocomplete (but are not yet functional withoutmakerlabEnabled). This PR changes Maker API blocks to only get folded in when makerlab is enabled.pixel/ledblocks.util.deepMergeConcatArraysutility (may become simpler in future version of immutable-js per mergeDeep clobbers List items; concatDeep isn't implemented immutable-js/immutable-js#406)Before
/p/applab
/p/makerlab
After
/p/applab
/p/makerlab