feat(config): add upgrade code for nodelibs-* deps #2120

frederikschubert · 2016-10-11T18:48:31Z

All github:jspm/nodelibs-* dependencies are being
replaced with npm:jspm-nodelibs-* dependencies.

frederikschubert · 2016-10-11T18:51:02Z

@guybedford I tried it locally but it did only change some of my nodelibs-* dependencies to the npm packages.

codecov-io · 2016-10-11T18:58:08Z

Current coverage is 34.59% (diff: 3.77%)

Merging #2120 into 0.17 will decrease coverage by 0.25%

@@               0.17      #2120   diff @@
==========================================
  Files            16         16          
  Lines          3891       3934    +43   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           1356       1361     +5   
- Misses         2535       2573    +38   
  Partials          0          0

Powered by Codecov. Last update 78da833...aada617

guybedford

Looks good - I'd be interested to hear if including peerDependencies gets the full upgrade path.

guybedford · 2016-10-19T14:54:05Z

lib/config/index.js

+    }
+  });
+
+  Object.keys(pjson.devDependencies).forEach(function (key) {


This probably needs peerDependencies as well?

(it wasn't necessary before because peerDependencies was only added in 0.17)

guybedford · 2016-10-19T14:56:46Z

lib/config/index.js

+  target.exactName = target.exactName.replace('github:jspm/nodelibs-', 'npm:jspm-nodelibs-');
+  target.exactNameEncoded = target.exactNameEncoded.replace('github:jspm/nodelibs-', 'npm:jspm-nodelibs-');
+  target.setVersion('0.2.0');
+  return target;


Rather than this manual modification, we can use target.setVersion, target.setRegistry and target.setPackage.

guybedford · 2016-10-19T14:57:11Z

lib/config/index.js

+        if (key == target.name.substr(21))
+          delete curMap[key];
+        else
+          installRequired = true;


This should probably just be at the top of the entire block?

I am not exactly sure what you mean here.

The original version of this code was designed to have two separate branches:

If the submap name matches the node core lib name (fs: github:jspm-nodelibs-fs@...) then it deletes the key entirely. This is for the beta upgrade path so we turn Node core dependencies from normal dependencies into peer dependencies.

If the submap name doesn't match (the else above in the original code) then update the target version (custom-fs-name: github:nodelibs-fs@...). That's a very rare case for jspm-only workflows but what this was designed for in the original code to handle this upgrade comprehensively.

In the adjusted code above installRequired is now set on the else path only, and target is always updated on all paths due to the lack of curly braces on the conditional.

So there are two problems here:

The code above is malformed, at least from a tabbing perspective.

We probably do need another flag to indicate the difference between the beta upgrade path and the nodelibs upgrade path. In the first case we want the original behaviour to be maintained. In the second case we just need to do replacement (the target update line below), and probably don't even need the conditional above.

Let me know if that makes sense.

Alright. I wasn't aware of this distinction.
How do you propose to detect the difference between the beta upgrade path and the nodelibs upgrade path? I am not familiar enough with the code base to come up with something reasonable.

guybedford · 2016-10-19T14:58:52Z

lib/config/index.js

@@ -74,6 +74,71 @@ exports.getLoaderConfig = function() {
  return cfg;
 };

+function updateNodelibTarget(target) {


There should probably be a second argument here - range as boolean. We can then use setVersion('^0.2.0) for the package.json dependencies to ensure we get semver upgrade paths. While the depMap set will be range: false.

All github:jspm/nodelibs-* dependencies are being replaced with npm:jspm-nodelibs-* dependencies.

guybedford · 2016-10-20T10:08:31Z

Thanks for the updates here. Were you able to test out the upgrade path again?

frederikschubert · 2016-10-20T10:09:50Z

Not yet. But will try to test it later today!

guybedford

That sort of structure should handle it I think.

guybedford · 2016-10-20T11:32:58Z

lib/config/index.js

@@ -74,6 +74,79 @@ exports.getLoaderConfig = function() {
  return cfg;
 };

+function updateNodelibTarget(target, range) {


function (target, range, upgrade)

I guess you meant

function processNodelibs(loader, pjson, upgrade)

?

guybedford · 2016-10-20T11:33:17Z

lib/config/index.js

-          delete config.pjson.devDependencies[key];
-        }
-      });
+      processNodelibs(config.loader, config.pjson);


processNodelibs(config.loader, config.pjson, true)

guybedford · 2016-10-20T11:33:27Z

lib/config/index.js

@@ -312,6 +343,16 @@ exports.load = function(prompts, allowNoConfig) {
    });
  }))
  .then(function(initInstalls) {
+    var installRequired = processNodelibs(config.loader, config.pjson);


processNodelibs(config.loader, config.pjson, false)

guybedford · 2016-10-20T12:05:11Z

lib/config/index.js

+    Object.keys(curMap).forEach(function (key) {
+      var target = curMap[key];
+
+      if (upgrade) {


I think this should be if (!upgrade).

guybedford · 2016-10-20T12:05:31Z

Yeah that's it, just check the one comment there.

frederikschubert · 2016-10-20T13:49:18Z

I just tested the changes but it does not seem to work yet.

guybedford · 2016-10-21T10:18:16Z

Ok, I can look at this, but it will just take me a while to find the time.

guybedford · 2016-10-25T14:17:44Z

👍

Merged manually, and made some minor corrections in ffadde5. The main issue I think was also bumping the npm registry cache version to ensure we don't use previously cached dependency values (jspm/npm@7e03fea).

frederikschubert · 2016-10-25T15:21:08Z

Great! 👍

guybedford · 2016-10-25T20:03:22Z

Another issue here was we missed the transitive deps - jspm/nodelibs-path@366c5b0. I think that's all the cases now.

frederikschubert force-pushed the 0.17 branch from 30a430f to 4ccab73 Compare October 11, 2016 18:52

guybedford requested changes Oct 19, 2016

View reviewed changes

guybedford reviewed Oct 19, 2016

View reviewed changes

frederikschubert added 2 commits October 20, 2016 11:20

feat(config): add upgrade code for nodelibs-* deps

f33afd4

All github:jspm/nodelibs-* dependencies are being replaced with npm:jspm-nodelibs-* dependencies.

Address comments

33c5b86

frederikschubert force-pushed the 0.17 branch from 4ccab73 to 33c5b86 Compare October 20, 2016 09:33

guybedford requested changes Oct 20, 2016

View reviewed changes

Make distinction between beta upgrade path and nodelibs upgrade

9f64cb5

guybedford reviewed Oct 20, 2016

View reviewed changes

Fix upgrade path

aada617

guybedford closed this Oct 25, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(config): add upgrade code for nodelibs-* deps #2120

feat(config): add upgrade code for nodelibs-* deps #2120

frederikschubert commented Oct 11, 2016

frederikschubert commented Oct 11, 2016

codecov-io commented Oct 11, 2016 •

edited

Loading

guybedford left a comment

guybedford Oct 19, 2016

guybedford Oct 19, 2016

frederikschubert Oct 20, 2016

guybedford Oct 19, 2016

frederikschubert Oct 20, 2016

guybedford Oct 19, 2016

frederikschubert Oct 20, 2016

guybedford Oct 20, 2016

frederikschubert Oct 20, 2016

guybedford Oct 19, 2016

frederikschubert Oct 20, 2016

guybedford commented Oct 20, 2016

frederikschubert commented Oct 20, 2016

guybedford left a comment

guybedford Oct 20, 2016

frederikschubert Oct 20, 2016

guybedford Oct 20, 2016

guybedford Oct 20, 2016

guybedford Oct 20, 2016

guybedford commented Oct 20, 2016

frederikschubert commented Oct 20, 2016

guybedford commented Oct 21, 2016

guybedford commented Oct 25, 2016

frederikschubert commented Oct 25, 2016

guybedford commented Oct 25, 2016

feat(config): add upgrade code for nodelibs-* deps #2120

feat(config): add upgrade code for nodelibs-* deps #2120

Conversation

frederikschubert commented Oct 11, 2016

frederikschubert commented Oct 11, 2016

codecov-io commented Oct 11, 2016 • edited Loading

Current coverage is 34.59% (diff: 3.77%)

guybedford 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

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

guybedford commented Oct 20, 2016

frederikschubert commented Oct 20, 2016

guybedford 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

guybedford commented Oct 20, 2016

frederikschubert commented Oct 20, 2016

guybedford commented Oct 21, 2016

guybedford commented Oct 25, 2016

frederikschubert commented Oct 25, 2016

guybedford commented Oct 25, 2016

codecov-io commented Oct 11, 2016 •

edited

Loading