CB-12142: Move windows-specific logic from cordova-common #207

matrosov-nikita · 2016-10-31T14:14:59Z

What does this PR do?

This PR moves windows-specific logic from cordova-common to cordova-windows. This logic is associated with demuxing 'package.appxmanifest' into relevant platform-specific appx manifests. Also auto-test was moved and edited according changes.

What testing has been done on this change?

Auto test

Checklist

Reported an issue in the JIRA database
Commit message follows the format: "CB-3232: (android) Fix bug with resolving file paths", where CB-xxxx is the JIRA ID & "android" is the platform affected.
Added automated test coverage as appropriate for this change.

daserge

LGTM apart the comments and assuming the tests will pass.
Do I get it right that it will require us to release cordova-common before platform release?

daserge · 2016-11-11T14:37:15Z

template/cordova/lib/PluginInfo.js

+        // Demux 'package.appxmanifest' into relevant platform-specific appx manifests.
+        // Only spend the cycles if there are version-specific plugin settings
+        if (changes.some(function(change) {
+                    return change.target === 'package.appxmanifest';


@matrosov-nikita could you explain the code change here from

changes.some(function(change) { return ((typeof change.versions !== 'undefined') || (typeof change.deviceTarget !== 'undefined')); }))

?

Changes w/ target 'package.appxmanifest' and w/o versions and deviceTarget used to demux into manifests in remove_plugin_changes and munge_helper methods. This logic is moved in PluginInfo.js (see also b101ded?diff=unified#diff-93a26ea6e269125a5b151c4176a3b42bR54).
Once windows-specific logic is completed there aren't any changes with 'package.appxmanifest' target.

In cordova-common we'll get ready changes and push them again. (see also https://github.com/apache/cordova-lib/blob/master/cordova-common/src/ConfigChanges/ConfigChanges.js#L365). So, I suppose, that we needn't release cordova-common before platform release.

OK, thank you!
Could you update the code so that it passes for node@0.x (as we a still supporting it)?

@daserge, i think this condition has been moved to line 50

daserge · 2016-11-11T14:47:07Z

spec/unit/ConfigChanges.spec.js

+        expect(windows81AppxManifest.parents['/Parent/Capabilities'][0].xml).toBe('<Capability Note="should-exist-for-all-appxmanifest-target-files" />');
+        expect(windows81AppxManifest.parents['/Parent/Capabilities'][1].xml).toBe('<Capability Note="should-exist-for-win81-win-and-phone" />');
+        expect(windows81AppxManifest.parents['/Parent/Capabilities'][2].xml).toBe('<Capability Note="should-exist-for-win81-win-only" />');
+        expect(windows81AppxManifest.parents['/Parent/Capabilities'][3].xml).toBe('<Capability Note="should-exist-for-win10-and-win81-win-and-phone" />');


Please add checks for capabilities count back - they could prevent situation when we have something extraneous.
expect(windows81AppxManifest.parents['/Parent/Capabilities'][0].count).toBe(3);

@daserge, done.

vladimir-kotikov · 2016-11-12T00:29:34Z

Do I get it right that it will require us to release cordova-common before platform release?

@matrosov-nikita, I think the idea was to make it so updated cordova-windows will continue to work w/ current cordova-common, right?

matrosov-nikita · 2016-11-13T14:22:40Z

@vladimir-kotikov, yes, you are right. Have you any questions or concerns about this matter?

daserge · 2016-11-14T07:33:33Z

spec/unit/ConfigChanges.spec.js

@@ -187,12 +187,14 @@ describe('generate_plugin_config_munge for windows project', function() {
        // 1 comes from versions="=8.1.0" + 1 from versions="=8.1.0" device-target="windows"
        expect(windows81AppxManifest.parents['/Parent/Capabilities'][0].xml).toBe('<Capability Note="should-exist-for-all-appxmanifest-target-files" />');
        expect(windows81AppxManifest.parents['/Parent/Capabilities'][1].xml).toBe('<Capability Note="should-exist-for-win81-win-and-phone" />');
+        expect(windows81AppxManifest.parents['/Parent/Capabilities'][1].count).toBe(2);


@matrosov-nikita is this a right element to check count?
Should not it be expect(windows81AppxManifest.parents['/Parent/Capabilities'].count).toBe(4);?
I know it was this way in the original code, but it just looks strange.

daserge · 2016-11-14T07:37:13Z

Also, @matrosov-nikita could you please create a Jira issue for tracking (http://issues.cordova.io), rename the PR title and add the commit prefix?

vladimir-kotikov · 2016-11-14T07:43:49Z

template/cordova/lib/ConfigChanges.js

+ * @return {[Object]} plugin's munge
+ */
+PlatformMunger.prototype.generate_plugin_config_munge = function (pluginInfo, vars, edit_config_changes) {
+     var self = this;


This doesn't seem to be requried - calling a prototype is not a closure so you can safely use this

vladimir-kotikov · 2016-11-14T07:54:19Z

template/cordova/lib/PluginInfo.js

+ */
+function PluginInfo(dirname) {
+    CommonPluginInfo.apply(this, arguments);
+    var parentGetConfigFiles = this.getConfigFiles;


Rather than saving this into a variable i'd propose to call ThisClass.super_.prototype.generate_plugin_config_munge method directly

vladimir-kotikov · 2016-11-14T07:56:24Z

template/cordova/lib/PluginInfo.js

+    var parentGetConfigFiles = this.getConfigFiles;
+    this.getConfigFiles = function(platform) {
+        var changes = parentGetConfigFiles(platform);
+        var editChanges = this.getEditConfigs(platform);


I don't think that we should do something with <edit-config> tags here. You'd probably need to override parent getEditConfigs method in a same way as getConfigFiles possibly sharing the similar logic between them.

vladimir-kotikov · 2016-11-14T07:59:43Z

template/cordova/lib/PluginInfo.js

+        }
+        // Demux 'package.appxmanifest' into relevant platform-specific appx manifests.
+        // Only spend the cycles if there are version-specific plugin settings
+        if (changes.some(function(change) {


Nit: could you please invert if condition here? Also it might be more convenient to assign this condition to intermediate variable, something like var hasManifestChanges = changes.some(function(change) { return change.target === 'package.appxmanifest' })

vladimir-kotikov · 2016-11-14T08:01:36Z

template/cordova/lib/PluginInfo.js

+        // Demux 'package.appxmanifest' into relevant platform-specific appx manifests.
+        // Only spend the cycles if there are version-specific plugin settings
+        if (changes.some(function(change) {
+                    return change.target === 'package.appxmanifest';


@daserge, i think this condition has been moved to line 50

vladimir-kotikov · 2016-11-14T08:05:54Z

template/cordova/lib/PluginInfo.js

+                    changes = changes.concat(substs.map(function(subst) {
+                        return Object.assign({}, change, {target: subst});
+                    }));
+                    return changes;


Nit: you can just return here - result of this function is not used anywhere

vladimir-kotikov · 2016-11-14T08:09:09Z

template/cordova/lib/PluginInfo.js

+In addition it overrides getConfigFiles method to use windows-specific logic
+ */
+function PluginInfo(dirname) {
+    CommonPluginInfo.apply(this, arguments);


Pls add a comment, clarifying why you're not using util.inherit to build a children class

vladimir-kotikov · 2016-11-14T08:10:04Z

template/cordova/lib/PluginInfo.js

+            var oldChanges = changes;
+            changes = [];
+
+            oldChanges.forEach(function(change, changeIndex) {


A general comment - can you please divide this function into smaller pieces, so it'd be easier to review and test?

codecov-io · 2016-11-14T10:42:53Z

Current coverage is 75.28% (diff: 100%)

Merging #207 into master will increase coverage by 0.63%

@@             master       #207   diff @@
==========================================
  Files            15         16     +1   
  Lines          2067       2120    +53   
  Methods         390        401    +11   
  Messages          0          0          
  Branches        403        410     +7   
==========================================
+ Hits           1543       1596    +53   
  Misses          524        524          
  Partials          0          0

Powered by Codecov. Last update 541c451...ba22817

matrosov-nikita · 2016-11-14T12:10:39Z

@vladimir-kotikov @daserge, I've created a Jira issue: https://issues.apache.org/jira/browse/CB-12142, fixed all your comments. Also I think it would be better to add prefix to commit after squashing. Pls, review.

vladimir-kotikov · 2016-11-14T14:17:29Z

template/cordova/lib/PluginInfo.js

+        return processChanges(configFiles);
+    };
+
+    function processChanges(changes) {


I'd suggest to move this out of PluginInfo - this way it'd be possible to use this function in tests (using rewire)

vladimir-kotikov · 2016-11-14T14:22:48Z

template/cordova/lib/PluginInfo.js

+        return changes;
+    }
+
+    function checkManifestChanges(changes) {


You're calling this only once - is there a real need to wrap this into function?

vladimir-kotikov · 2016-11-14T14:23:03Z

template/cordova/lib/PluginInfo.js

+            return change.target === 'package.appxmanifest';
+        });
+    }
+    function hasChangeVersion(change) {


vladimir-kotikov · 2016-11-14T14:23:09Z

template/cordova/lib/PluginInfo.js

+        return (typeof change.versions !== 'undefined');
+    }
+
+    function hasChangeTarget(change) {


vladimir-kotikov · 2016-11-14T14:29:29Z

template/cordova/lib/PluginInfo.js

+                }
+
+                var targetDeviceSet = getDeviceTargetSetForChange(change);
+                changes = replaceChangeTarget(targetDeviceSet, change, changes);


Consider renaming replaceChangeTarget to something more appropriate as it doesn't really replace anything in the original array

vladimir-kotikov · 2016-11-14T14:31:41Z

template/cordova/lib/PluginInfo.js

+
+    // This is a local function that creates the new replacement representing the
+    // mutation.  Used to save code further down.
+    function createReplacement(manifestFile, originalChange) {


This looks very similar to createChangeWithNewTarget function - consider merging into one function

This logic is associated with demuxing 'package.appxmanifest' into relevant platform-specific appx manifests. Also auto-test was moved and edited according changes

vladimir-kotikov · 2016-11-17T14:32:06Z

Looks good. Let's wait for tomorrow and merge it.

matrosov-nikita mentioned this pull request Nov 1, 2016

Move windows-specific logic to cordova-windows apache/cordova-lib#505

Closed

3 tasks

daserge suggested changes Nov 11, 2016

View reviewed changes

daserge reviewed Nov 14, 2016

View reviewed changes

vladimir-kotikov suggested changes Nov 14, 2016

View reviewed changes

matrosov-nikita force-pushed the remove-windows-logic branch from 6019414 to 32c9f82 Compare November 14, 2016 10:43

matrosov-nikita changed the title ~~Move windows-specific logic from cordova-common~~ CB-12142: Move windows-specific logic from cordova-common Nov 14, 2016

vladimir-kotikov suggested changes Nov 14, 2016

View reviewed changes

matrosov-nikita force-pushed the remove-windows-logic branch 11 times, most recently from 87d886a to 600ac0b Compare November 16, 2016 11:41

CB-12142: Move windows-specific logic from cordova-common

8753c42

This logic is associated with demuxing 'package.appxmanifest' into relevant platform-specific appx manifests. Also auto-test was moved and edited according changes

matrosov-nikita force-pushed the remove-windows-logic branch 3 times, most recently from 034a6b3 to 519016f Compare November 17, 2016 11:13

Fix pattern for extracting capabilities names

00b6fa7

matrosov-nikita force-pushed the remove-windows-logic branch from 141e52c to 00b6fa7 Compare November 17, 2016 13:08

Do not ignore already prefixed capabilities at plugin add/rm

ba22817

asfgit closed this in dacf70e Nov 18, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CB-12142: Move windows-specific logic from cordova-common #207

CB-12142: Move windows-specific logic from cordova-common #207

matrosov-nikita commented Oct 31, 2016

daserge left a comment

daserge Nov 11, 2016

matrosov-nikita Nov 13, 2016

matrosov-nikita Nov 13, 2016

daserge Nov 14, 2016

vladimir-kotikov Nov 14, 2016

daserge Nov 11, 2016

matrosov-nikita Nov 13, 2016

vladimir-kotikov commented Nov 12, 2016

matrosov-nikita commented Nov 13, 2016

daserge Nov 14, 2016

daserge commented Nov 14, 2016

vladimir-kotikov Nov 14, 2016

vladimir-kotikov Nov 14, 2016

vladimir-kotikov Nov 14, 2016

vladimir-kotikov Nov 14, 2016

vladimir-kotikov Nov 14, 2016

vladimir-kotikov Nov 14, 2016

vladimir-kotikov Nov 14, 2016

vladimir-kotikov Nov 14, 2016

codecov-io commented Nov 14, 2016 •

edited

matrosov-nikita commented Nov 14, 2016

vladimir-kotikov Nov 14, 2016

vladimir-kotikov Nov 14, 2016

vladimir-kotikov Nov 14, 2016

vladimir-kotikov Nov 14, 2016

vladimir-kotikov Nov 14, 2016

vladimir-kotikov Nov 14, 2016

vladimir-kotikov commented Nov 17, 2016

CB-12142: Move windows-specific logic from cordova-common #207

CB-12142: Move windows-specific logic from cordova-common #207

Conversation

matrosov-nikita commented Oct 31, 2016

What does this PR do?

What testing has been done on this change?

Checklist

daserge left a 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

vladimir-kotikov commented Nov 12, 2016

matrosov-nikita commented Nov 13, 2016

Choose a reason for hiding this comment

daserge commented Nov 14, 2016

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

codecov-io commented Nov 14, 2016 • edited

Current coverage is 75.28% (diff: 100%)

matrosov-nikita commented Nov 14, 2016

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

vladimir-kotikov commented Nov 17, 2016

codecov-io commented Nov 14, 2016 •

edited