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.

Sign up for GitHub

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

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

Closed
wants to merge 4 commits into from
Closed

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

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f33afd4
feat(config): add upgrade code for nodelibs-* deps
frederikschubert Oct 11, 2016
33c5b86
Address comments
frederikschubert Oct 20, 2016
9f64cb5
Make distinction between beta upgrade path and nodelibs upgrade
frederikschubert Oct 20, 2016
aada617
Fix upgrade path
frederikschubert Oct 20, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
127 changes: 84 additions & 43 deletions lib/config/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,79 @@ exports.getLoaderConfig = function() {
return cfg;
};

function updateNodelibTarget(target, range) {
Copy link
Member

Choose a reason for hiding this comment

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

function (target, range, upgrade)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess you meant

function processNodelibs(loader, pjson, upgrade)

?

target.setRegistry('npm');
target.setVersion(range ? '^0.2.0' : '0.2.0');
target.setPackage(target.package.replace('jspm/nodelibs-', 'jspm-nodelibs-'));
return target;
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

}

function processNodelibs(loader, pjson) {
var installRequired = false;
Object.keys(loader.baseMap).forEach(function (key) {
var target = loader.baseMap[key];

if (target.name.substr(0, 21) == 'github:jspm/nodelibs-') {
target = updateNodelibTarget(target, false);
installRequired = true;
}
});

Object.keys(loader.depMap).forEach(function (key) {
var curMap = loader.depMap[key];

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

if (target.name.substr(0, 21) == 'github:jspm/nodelibs-') {
if (key == target.name.substr(21))
delete curMap[key];
else
installRequired = true;
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not exactly sure what you mean here.

Copy link
Member

Choose a reason for hiding this comment

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

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:

  1. The code above is malformed, at least from a tabbing perspective.
  2. 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

target = updateNodelibTarget(target, false);
}
});
});

Object.keys(pjson.dependencies).forEach(function (key) {
var target = pjson.dependencies[key];

if (target.name.substr(0, 21) == 'github:jspm/nodelibs-') {
target = updateNodelibTarget(target, true);
installRequired = true;

pjson.peerDependencies[key] = target;
delete pjson.dependencies[key];
}
});

Object.keys(pjson.devDependencies).forEach(function (key) {
Copy link
Member

Choose a reason for hiding this comment

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

This probably needs peerDependencies as well?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

var target = pjson.devDependencies[key];

if (target.name.substr(0, 21) == 'github:jspm/nodelibs-') {
target = updateNodelibTarget(target, true);
installRequired = true;

pjson.peerDependencies[key] = target;
delete pjson.devDependencies[key];
}
});

Object.keys(pjson.peerDependencies).forEach(function (key) {
var target = pjson.peerDependencies[key];

if (target.name.substr(0, 21) == 'github:jspm/nodelibs-') {
target = updateNodelibTarget(target, true);
installRequired = true;

pjson.peerDependencies[key] = target;
delete pjson.peerDependencies[key];
}
});

return installRequired;
}

var loadPromise;
exports.loaded = false;
exports.allowNoConfig = false;
Expand Down Expand Up @@ -176,49 +249,7 @@ exports.load = function(prompts, allowNoConfig) {

ui.log('info', 'Upgrading jspm 0.16 Node core libraries to jspm 0.17 universal implementations...\n');

Object.keys(config.loader.baseMap).forEach(function(key) {
var target = config.loader.baseMap[key];

if (target.name.substr(0, 21) == 'github:jspm/nodelibs-')
target.setVersion('0.2.0-alpha');
});

Object.keys(config.loader.depMap).forEach(function(key) {
var curMap = config.loader.depMap[key];

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

if (target.name.substr(0, 21) == 'github:jspm/nodelibs-') {
if (key == target.name.substr(21))
delete curMap[key];
else
target.setVersion('0.2.0-alpha');
}
});
});

Object.keys(config.pjson.dependencies).forEach(function(key) {
var target = config.pjson.dependencies[key];

if (target.name.substr(0, 21) == 'github:jspm/nodelibs-') {
target.setVersion('^0.2.0-alpha');

config.pjson.peerDependencies[key] = target;
delete config.pjson.dependencies[key];
}
});

Object.keys(config.pjson.devDependencies).forEach(function(key) {
var target = config.pjson.devDependencies[key];

if (target.name.substr(0, 21) == 'github:jspm/nodelibs-') {
target.setVersion('^0.2.0-alpha');

config.pjson.peerDependencies[key] = target;
delete config.pjson.devDependencies[key];
}
});
processNodelibs(config.loader, config.pjson);
Copy link
Member

Choose a reason for hiding this comment

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

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


ui.log('info', 'Checking all overrides into the package.json file to ensure reproducibility independent of the jspm registry...\n');

Expand Down Expand Up @@ -311,6 +342,16 @@ exports.load = function(prompts, allowNoConfig) {
});
});
}))
.then(function(initInstalls) {
var installRequired = processNodelibs(config.loader, config.pjson);
Copy link
Member

Choose a reason for hiding this comment

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

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

if (installRequired) {
return require('../install').install(true).then(function () {
return initInstalls;
});
} else {
return initInstalls;
}
})
.then(function(initInstalls) {
config.loaded = true;

Expand Down