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.

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

Make it possible tns to use Yarn as the package manager #2737

Closed
dtopuzov opened this issue Apr 24, 2017 · 26 comments
Closed

Make it possible tns to use Yarn as the package manager #2737

dtopuzov opened this issue Apr 24, 2017 · 26 comments

Comments

@dtopuzov
Copy link
Contributor

From @ugultopu on April 24, 2017 17:26

Is it possible to configure Nativescript CLI tool to use Yarn as the package manager?

Copied from original issue: NativeScript/NativeScript#4058

@dtopuzov
Copy link
Contributor Author

dtopuzov commented May 4, 2017

No it is not possible at the moment.

@dtopuzov dtopuzov closed this as completed May 4, 2017
@ishitatsuyuki
Copy link

Wait how impossible? This should be a feature request.

@dtopuzov
Copy link
Contributor Author

dtopuzov commented May 5, 2017

Reopen, title update and label question changes with feature.
We will be happy if someone wants to contribute on this feature.

@dtopuzov dtopuzov reopened this May 5, 2017
@dtopuzov dtopuzov changed the title Is it possible to make tns use Yarn as the package manager? Make it possible tns to use Yarn as the package manager May 5, 2017
@ibrahimab
Copy link

Any word on this? Any milestones so people can help by contributing ?

@rosen-vladimirov
Copy link
Contributor

Hey @ibrahimab ,
Currently we are working on other features and this one is not in our short-term plans. That's why we've added the "help wanted" label. We'll be glad to review PRs adding this functionality.
In order to make it easier for contribution, I would like to share some insides how CLI works and where npm is used. NativeScript CLI uses dependency injection which allows easier bootstrap and testing. More information about this can be found in CLI's submodule.
CLI uses npm for project creation, runtimes installation, plugin manipulation, etc. Basically all npm related operations are in a single file: nodePackageManager.
There are different ways to add support for yarn, but if I have to do it, I would try the following approach:

  1. Add a new service for example yarnPackageManager (and register it as $yarn) that implements the same interface as nodePackageManager: INodePackageManager (and maybe rename the interface later).
  2. Add a new service that is wrapper for both $npm and $yarn. This service must have a private method that decides if $yarn or $npm will be used. The service must implement the same interface (INodePackageManage). This service can be injected with name $packageManager.
  3. Replace all occurrences of $npm with $packageManager.
  4. Implement a way for users to switch to yarn. One possible solution is to add a --yarn flag, so in case users want to use yarn, they'll have to pass it. In the private method mentioned in point 2. (inside $packageManager), you can read the value of $options.yarn - if it is true, the user had passed it, so the method will return $yarn. In fact, in code this should look similar to this one (in icenium-cli). The $packageManager will have wrappers for all methods, based on user input it will decide if $npm or $yarn should be called.
    In case we do not want users to pass --yarn on each command, we can add a command that persists the value in the users settings. The new command can be something like tns package manager set yarn or tns package manager set npm. The command will call $userSettingsService.saveSetting("packageManager", "yarn") for example. The $packageManager will be able to read the value and based on it, it will resolve either yarn or npm.

Of course, this is just an idea for implementation and any other suggestions and implementations are welcome.
Hope this information will allow anyone to try implementing this feature.

@ibrahimab
Copy link

Hi @rosen-vladimirov thank you for your response. I want to take this up. How would this work? I want to setup milestones, tasks, issues etc. Do I do this on this repository or link to my own fork?
I'm leaning more towards 2 and 3. I want a way that the system detects someone changed to yarn without them changing anything in their package.json. I am also more in favor of setting yarn as the default package manager. Any input on what I just said?

@rosen-vladimirov
Copy link
Contributor

Hey @ibrahimab ,
I'm not sure how you'll detect that someone has started using yarn instead of npm without specifying it. For example, I have yarn installed, but I'm using npm in my daily work. I would suggest using a command in CLI that sets the value of the selected package manager in the user settings. These user settings are persisted even when you install new version of CLI.
Regarding the workflow when working on this feature - you'll have to fork this repository and create a branch in your own fork. You can work in the branch and once you are ready with the implementation, you can create a PR in this repository. You can check the following resources that might help you during contribution:

Regarding the tasks and issues, I would suggest to add them here, as comments, so anyone who wants to help, can take a look at the current status of the task. The other option is to create the issues in your own fork and link them in the comment here.

@rosen-vladimirov
Copy link
Contributor

Hey @ibrahimab ,
Have you tried working on this feature? Do you need more information how to get started?

@mflor35
Copy link
Contributor

mflor35 commented Oct 5, 2018

@dcarrot2 and myself are looking into implementing this. We a little frustrated on how npm does not uses package-lock.json to lock dependencies. We are also looking into take advantage of yarn's improved performance.

@dcarrot2
Copy link

dcarrot2 commented Oct 5, 2018

@rosen-vladimirov A few questions we have.

  1. Are the steps you outlined above on Feb 15 still a reasonable approach for us to take?

  2. We noticed you introduced NpmService about 15 days ago--is this a replacement for node-package-manager?

Thanks!

@rosen-vladimirov
Copy link
Contributor

rosen-vladimirov commented Oct 5, 2018

Hey guys, I'm happy you're gonna try implementing this feature. I'll be glad to help you and review any code or idea you have.
Regarding your questions:

  1. Yes, the suggestion is still valid. In case I had to implement the feature, I would take exactly the described approach.
  2. No, this service came from removing the submodule, which was shared with other products. We'll soon remove the NpmService

@rosen-vladimirov
Copy link
Contributor

Hey @dcarrot2 , @mflor35 ,
Have you tried implementing this feature? I'll be glad to help you in case you have any questions.

@dcarrot2
Copy link

Yes, we're underway implementing it, but @mflor35 and I are traveling this week. We've hammered most of yarn's implementation except install. We noticed there isn't a dry run flag that we can pass to yarn, and yarn streams it's json format as the installation occurs--for all events in the routine. We're roughly thinking of the following approach to address installation and fulfill the adopted interface.

  1. Listen to the stdout stream for the child process calling yarn --json and attempt to json parse the passed string.
  2. If successfully parsed, attempt to cast it to yarn's interface of an installation event.
  3. If successfully casted, check if the package installed in the event matches the given package name, and return the name and version of the package in the install method.

Thoughts @rosen-vladimirov?

@rosen-vladimirov
Copy link
Contributor

Hey @dcarrot2 ,
The idea seems correct and I think it will work. However, I would suggest another approach, which should be easier for implementation - recently we've added a $pacoteService that uses the great pacote package under the hood. We've planned to remove the call with dry run flag, as it is unstable and its behavior varies on different npm and OS versions.
So instead of parsing the json's, you can just call $pacoteService.manifest and get the real name and version of the installed package from there.
In case you are trying to bypass the case where CLI calls npm install on the project level, I think the dry run result there is not used anywhere in the code, so you can safely skip this call.

@BenLorantfy
Copy link

@dcarrot2 Are you implementing this in a way that would enable yarn's workspaces feature? That would allow NativeScript apps to be used in monorepos. Not sure if this requires other changes to NativeScript though

@dcarrot2
Copy link

Thanks for the guidance @rosen-vladimirov! Will take a look at pacote--looks like that should simply things greatly. @BenLorantfy Will scope that out once we've hammered out this feature request.

@Fatme
Copy link
Contributor

Fatme commented Nov 12, 2018

The settings for selected package manager is persisted in user-settings.json file. There are tns package manager set and tns package manager get commands to respectively set and get the value.

Acceptance criteria:

  • should check existing automation tests are passed
  • should check tns package-manager set npm
  • should check tns package-manager set yarn
  • should check tns package-manager get
  • should check create command when yarn is set as package manager in user-settings.json
  • should check platform add command when yarn is set as package manager in user-settings.json
  • should check prepare command when yarn is set as package manager in user-settings.json
  • should check build command when yarn is set as package manager in user-settings.json
  • should check run command when yarn is set as package manager in user-settings.json
  • should check deploy command when yarn is set as package manager in user-settings.json
  • should check debug command when yarn is set as package manager in user-settings.json
  • should check plugin add command when yarn is set as package manager in user-settings.json
  • should check plugin remove command when yarn is set as package manager in user-settings.json
  • should check preview command when yarn is set as package manager in user-settings.json
  • should check all above commands when yarn is passed as option to the command (--yarn)

Sorry, something went wrong.

@nandin-borjigin
Copy link

I built the master branch locally and tried it out. Package installation seems to be working as expected. But when I run ts run, webpack compiles successfully but the whole process failed at some phase likely because of package resolution problem.
image

As I'm using nativescript for the first time, I can only guess from the console output that there is some package resolution defect in prepareJSApp phase.

I setup the project using vue-cli following quick start. One thing might worth noting is that I created the project inside a pre-existing lerna monorepo and common node dependencies are hoisted to the root node_modules.

file system hierarchy:

my-project/   ---- monorepo
    node_modules/ --- root node_modules
    packages/
        libero-mobile/  --- newly created nativescript project
        other-sub-projects/

@nandin-borjigin
Copy link

nandin-borjigin commented Nov 14, 2018

After digging into the source code, I found that PluginService only searches in the <ProjectDir>/node_modules and then causes the above warning

@Fatme
Copy link
Contributor

Fatme commented Nov 14, 2018

@Nandiin,

I see you're using yarn and wonder if the problem is reproducible when npm is set as package manager?

@nandin-borjigin
Copy link

nandin-borjigin commented Nov 14, 2018

@Fatme It is yarn-specific.

Since npm installs all dependencies just under the project directory (as it doesn't know anything about yarn workspace and monorepo), the PluginService can successfully read the dependencies from <ProjectDir>/node_modules.

Yarn, however, collects dependencies to the root node_modules when using workspace feature thus causing dependencies to not exist in the <ProjectDir>/node_modules (Instead, dependecies are collected to <ProjectDir>/../../node_modulesin my case, which is the root of the monorepo).

I suppose these functions (there may be some others) of the PluginService need to be modified to support yarn workspace feature to fully support yarn as package manager

image


ps: There was a markdown formatting error in my previous comment which causes <ProjectDir>/node_modules to be displayed as /node_modules and I have fixed it. I'm apologizing here if it confounded you.

@nandin-borjigin
Copy link

Technically speaking, module resolution algorithm of nodejs require natively supports searching in ancestor directories and yarn workspace just exploited this feature.

@nandin-borjigin
Copy link

nandin-borjigin commented Nov 14, 2018

Changing getPackageJsonFilePathForModule function like below helps tns run survive previous error and now I'm confronting another 'file cannot be found' error thrown from the xcode.

getPackageJsonFilePathForModule(moduleName, projectDir) {
    return require.resolve(path.join(moduleName, 'package.json'), { paths: [projectDir] })
}

Still working on it.

@nandin-borjigin
Copy link

nandin-borjigin commented Nov 15, 2018

After solving the errors one by one, I'm finally at a stage where a fresh new project created by vue init nativescript-vue/vue-cli-template command is successfully running on my iOS simulator.

Although I'd like to make a PR, I cannot make the test pass for now. Because I typed these tiral-and-error-codes by solving the errors one after another, instead of getting a full picture of the codebase first and solving it systematically, I do not really have the overall understanding to be able to modify the test to make it pass. And it's also very likely to have some corner cases omitted.

So, I'm attaching the diff here and hoping that someone who is more capable to make the PR.

@Fatme Would you mind to take a glance ?

diff

diff --git a/lib/services/plugins-service.ts b/lib/services/plugins-service.ts
index f18a9e8..e829e4b 100644
--- a/lib/services/plugins-service.ts
+++ b/lib/services/plugins-service.ts
@@ -2,6 +2,7 @@ import * as path from "path";
 import * as shelljs from "shelljs";
 import * as semver from "semver";
 import * as constants from "../constants";
+const walkup = require("node-walkup");
 
 export class PluginsService implements IPluginsService {
 	private static INSTALL_COMMAND_NAME = "install";
@@ -164,18 +165,32 @@ export class PluginsService implements IPluginsService {
 		}
 	}
 
+	private async getAllInstalledDependencies (projectData: IProjectData) {
+		// walk through ancestor directories and collect 'node_modules' paths
+		const nodeModulesPaths = await new Promise<string[]>((resolve, reject) => {
+			walkup('node_modules', { cwd: projectData.projectDir }, (err: any, matches: { dir: string, files: string[] }[]) =>
+				err ? reject(err) : resolve(matches.map(match => path.join(match.dir, 'node_modules')))
+			);
+		});
+
+		// get dependencies in one specific node_modules directory
+		const getDepsIn = (dir: string): (string | string[])[] => {
+			return this.$fs.readDirectory(dir)
+				.map(dep => {
+					if (!dep.startsWith('@')) { return dep; } // return directly if it's normal module
+					const scopedDir = path.join(dir, dep); // scoped module
+					return this.$fs.readDirectory(scopedDir).map(scopedDep => `${dep}/${scopedDep}`);
+				});
+		};
+		return _(nodeModulesPaths) // [ '/path/to/project/node_modules', '/path/to/project/packages/sub-project/node_modules' ]
+			.map(getDepsIn) // [ ['moduleA', 'moduleB'], ['moduleC', ['@d/moduleE', '@d/moduleF'] ], ['moduleB'] ]
+			.flattenDeep<string>() // [ 'moduleA', 'moduleB', 'moduleC', '@d/moduleE', '@d/moduleF', 'moduleB' ]
+			.uniq() // [ 'moduleA', 'moduleB', 'moduleC', '@d/moduleE', '@d/moduleF' ]
+			.value();
+	}
+
 	public async ensureAllDependenciesAreInstalled(projectData: IProjectData): Promise<void> {
-		let installedDependencies = this.$fs.exists(this.getNodeModulesPath(projectData.projectDir)) ? this.$fs.readDirectory(this.getNodeModulesPath(projectData.projectDir)) : [];
-		// Scoped dependencies are not on the root of node_modules,
-		// so we have to list the contents of all directories, starting with @
-		// and add them to installed dependencies, so we can apply correct comparison against package.json's dependencies.
-		_(installedDependencies)
-			.filter(dependencyName => _.startsWith(dependencyName, "@"))
-			.each(scopedDependencyDir => {
-				const contents = this.$fs.readDirectory(path.join(this.getNodeModulesPath(projectData.projectDir), scopedDependencyDir));
-				installedDependencies = installedDependencies.concat(contents.map(dependencyName => `${scopedDependencyDir}/${dependencyName}`));
-			});
-
+		const installedDependencies = await this.getAllInstalledDependencies(projectData);
 		const packageJsonContent = this.$fs.readJson(this.getPackageJsonFilePath(projectData.projectDir));
 		const allDependencies = _.keys(packageJsonContent.dependencies).concat(_.keys(packageJsonContent.devDependencies));
 		const notInstalledDependencies = _.difference(allDependencies, installedDependencies);
@@ -228,7 +243,7 @@ export class PluginsService implements IPluginsService {
 	}
 
 	private getPackageJsonFilePathForModule(moduleName: string, projectDir: string): string {
-		return path.join(this.getNodeModulesPath(projectDir), moduleName, "package.json");
+		return require.resolve(path.join(moduleName, 'package.json'), { paths: [projectDir] });
 	}
 
 	private getDependencies(projectDir: string): string[] {
diff --git a/lib/tools/node-modules/node-modules-dependencies-builder.ts b/lib/tools/node-modules/node-modules-dependencies-builder.ts
index 53dd1bc..e9189ab 100644
--- a/lib/tools/node-modules/node-modules-dependencies-builder.ts
+++ b/lib/tools/node-modules/node-modules-dependencies-builder.ts
@@ -1,5 +1,5 @@
 import * as path from "path";
-import { NODE_MODULES_FOLDER_NAME, PACKAGE_JSON_FILE_NAME } from "../../constants";
+import { PACKAGE_JSON_FILE_NAME } from "../../constants";
 
 interface IDependencyDescription {
 	parent: IDependencyDescription;
@@ -12,7 +12,6 @@ export class NodeModulesDependenciesBuilder implements INodeModulesDependenciesB
 	public constructor(private $fs: IFileSystem) { }
 
 	public getProductionDependencies(projectPath: string): IDependencyData[] {
-		const rootNodeModulesPath = path.join(projectPath, NODE_MODULES_FOLDER_NAME);
 		const projectPackageJsonPath = path.join(projectPath, PACKAGE_JSON_FILE_NAME);
 		const packageJsonContent = this.$fs.readJson(projectPackageJsonPath);
 		const dependencies = packageJsonContent && packageJsonContent.dependencies;
@@ -29,7 +28,7 @@ export class NodeModulesDependenciesBuilder implements INodeModulesDependenciesB
 
 		while (queue.length) {
 			const currentModule = queue.shift();
-			const resolvedDependency = this.findModule(rootNodeModulesPath, currentModule, resolvedDependencies);
+			const resolvedDependency = this.findModule(currentModule);
 
 			if (resolvedDependency && !_.some(resolvedDependencies, r => r.directory === resolvedDependency.directory)) {
 				_.each(resolvedDependency.dependencies, d => {
@@ -53,40 +52,17 @@ export class NodeModulesDependenciesBuilder implements INodeModulesDependenciesB
 		return resolvedDependencies;
 	}
 
-	private findModule(rootNodeModulesPath: string, depDescription: IDependencyDescription, resolvedDependencies: IDependencyData[]): IDependencyData {
-		let modulePath = path.join(depDescription.parentDir, NODE_MODULES_FOLDER_NAME, depDescription.name); // node_modules/parent/node_modules/<package>
-		const rootModulesPath = path.join(rootNodeModulesPath, depDescription.name);
-		let depthInNodeModules = depDescription.depth;
-
-		if (!this.moduleExists(modulePath)) {
-
-			let moduleExists = false;
-			let parent = depDescription.parent;
-
-			while (parent && !moduleExists) {
-				modulePath = path.join(depDescription.parent.parentDir, NODE_MODULES_FOLDER_NAME, depDescription.name);
-				moduleExists = this.moduleExists(modulePath);
-				if (!moduleExists)  {
-					parent = parent.parent;
-				}
-			}
-
-			if (!moduleExists) {
-				modulePath = rootModulesPath; // /node_modules/<package>
-				if (!this.moduleExists(modulePath)) {
-					return null;
-				}
-			}
-
-			depthInNodeModules = 0;
-		}
-
-		if (_.some(resolvedDependencies, r => r.name === depDescription.name && r.directory === modulePath)) {
+	private findModule(depDescription: IDependencyDescription): IDependencyData {
+		try {
+			const pkgJsonName = path.join(depDescription.name, 'package.json');
+			// resolve package.json instead of module name
+			// because require.resolve('some-module') would
+			// return the file specified by "main" key of the package.json and it's module-dependent and thus impossible to exploit
+			const pkgJsonPath = require.resolve(pkgJsonName, { paths: [depDescription.parentDir] });
+			return this.getDependencyData(depDescription.name, path.dirname(pkgJsonPath), depDescription.depth);
+		} catch (e) {
 			return null;
-
 		}
-
-		return this.getDependencyData(depDescription.name, modulePath, depthInNodeModules);
 	}
 
 	private getDependencyData(name: string, directory: string, depth: number): IDependencyData {
@@ -113,19 +89,6 @@ export class NodeModulesDependenciesBuilder implements INodeModulesDependenciesB
 
 		return null;
 	}
-
-	private moduleExists(modulePath: string): boolean {
-		try {
-			let modulePathLsStat = this.$fs.getLsStats(modulePath);
-			if (modulePathLsStat.isSymbolicLink()) {
-				modulePathLsStat = this.$fs.getLsStats(this.$fs.realpath(modulePath));
-			}
-
-			return modulePathLsStat.isDirectory();
-		} catch (e) {
-			return false;
-		}
-	}
 }
 
 $injector.register("nodeModulesDependenciesBuilder", NodeModulesDependenciesBuilder);
diff --git a/package.json b/package.json
index 40b7466..4e85613 100644
--- a/package.json
+++ b/package.json
@@ -58,6 +58,7 @@
     "mute-stream": "0.0.5",
     "nativescript-doctor": "1.6.0",
     "nativescript-preview-sdk": "0.3.0",
+    "node-walkup": "^1.1.1",
     "open": "0.0.5",
     "ora": "2.0.0",
     "osenv": "0.1.3",

@Fatme Fatme self-assigned this Nov 15, 2018
@nandin-borjigin
Copy link

nandin-borjigin commented Nov 15, 2018

As a side note: I checked the source code of the node-walkup and think that it's vulnerable on windows. It only examines if a path.dirname(directory) is one of ["", path.sep, "."](see here) to decide whether or not to exit the recursion. But path.dirname("C:\\") returns "C:\\" on windows and that might cause an infinite loop. However, it's my in-mind analysis and haven't tried on a real machine. Please consider providing a custom implementation of 'walkup' operation.

@Fatme
Copy link
Contributor

Fatme commented Nov 15, 2018

@Nandiin,

Thank you very much for sharing the code and for your effort!! It's amazing! We really appreciate it!

We're working on important updates this week and will not be able to review your code. After the release we'll consider all the possibilities and the effort we need to finish your work and make the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests