Skip to content

Install karma peer dependencies on test init #2693

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

Merged
merged 3 commits into from
Apr 25, 2017
Merged

Install karma peer dependencies on test init #2693

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3299f82
implicitly install karma-<testingFramework>'s peer dependencies on te…
petekanev Apr 5, 2017
0e89909
add exception handling when installing packages already present in th…
petekanev Apr 6, 2017
55f9806
log warning instead of throwing errors when a package's name couldn't…
petekanev Apr 10, 2017
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
19 changes: 19 additions & 0 deletions lib/commands/test-init.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,25 @@ class TestInitCommand implements ICommand {
'save-dev': true,
optional: false,
});

const modulePath = path.join(projectDir, "node_modules", mod);
const modulePackageJsonPath = path.join(modulePath, "package.json");
const modulePackageJsonContent = this.$fs.readJson(modulePackageJsonPath);
const modulePeerDependencies = modulePackageJsonContent.peerDependencies || {};

for (let peerDependency in modulePeerDependencies) {
let dependencyVersion = modulePeerDependencies[peerDependency] || "*";

// catch errors when a peerDependency is already installed
// e.g karma is installed; karma-jasmine depends on karma and will try to install it again
Copy link
Contributor

Choose a reason for hiding this comment

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

If it's already installed, why would it fail?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The node-package-manager's install() that I used will fail the build - https://github.com/NativeScript/nativescript-cli/blob/master/lib/node-package-manager.ts#L78

I am unaware why that check is performed, even though the npm package is installed anyway at an earlier point - https://github.com/NativeScript/nativescript-cli/blob/master/lib/node-package-manager.ts#L42.

@rosen-vladimirov can you say what the reasons for that are?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not really sure why there's an error there. Maybe it should be just warning. The code there is trying to find the name of the installed package based on the difference in package.json before installing the package and after that. In case the package is already installed, there will be no difference in package.json, so we are unable to determine the real name of the installed package (in case you are installing from github url or tarball, you do not know the real name of the package).

Copy link
Contributor

Choose a reason for hiding this comment

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

As the comment says, in case the dependency karma is already installed, the current implementation will reinstall it, but it may also change its version, which I do not think is expected. For example in case I have installed karma: 1.6.0, but the karma-jasmine has peerDependency: "karma":"~0.9", I'll end-up with the version ~0.9. Can we skip the installation of dependencies in case we already have the same dependency installed.

try {
await this.$npm.install(`${peerDependency}@${dependencyVersion}`, projectDir, {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should have the dependencyVersion in quotes, so that it works for dependencies defined like this:

"karma": "1.x || ",
"karma-chrome-launcher": "1.x || ~0.2.2",
"karma-firefox-launcher": "1.x || ~0.1.7",

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Explicitly adding quotes actually confused the npm.install call. The "1.x || ~0.2.2" style versions seem to work just alright

Copy link
Contributor

Choose a reason for hiding this comment

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

You can use the npm-installation-manager and npmInstall in order to not have duplicated npm.install statements in our code and avoid errors as the one above

'save-dev': true
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we want to save it to devDependencies? If the entry point is always the test init there's no need to do this?

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 don't understand your point. Testing frameworks should be installed as devDependencies so they are not included in built projects, but so that they may still be installed subsequently, for example after removing node_modules.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can disregard this. I didn't see the reason to duplicate them from peerDependencies - they're already present and installed from their presence there. Why add more changes to the package.json

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because otherwise the next time you decide to reinstall node_modules you would have to run test init again to ensure the testing framework previously used is installed.

});
} catch (e) {
this.$logger.error(e.message);
}
}
}

await this.$pluginsService.add('nativescript-unit-test-runner', this.$projectData);
Expand Down
4 changes: 2 additions & 2 deletions lib/node-package-manager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,10 +76,10 @@ export class NodePackageManager implements INodePackageManager {
let diff = dependencyDiff.concat(devDependencyDiff);

if (diff.length <= 0 && dependenciesBefore.length === dependenciesAfter.length && packageName !== pathToSave) {
this.$errors.failWithoutHelp(`The plugin ${packageName} is already installed`);
this.$logger.warn(`The plugin ${packageName} is already installed`);
}
if (diff.length <= 0 && dependenciesBefore.length !== dependenciesAfter.length) {
this.$errors.failWithoutHelp(`Couldn't install package correctly`);
this.$logger.warn(`Couldn't install package ${packageName} correctly`);
}

return diff;
Expand Down