feat() - [Yarn Support - Part 1 and 2] Yarn and Package Manager implementation #4050

mflor35 · 2018-10-22T23:04:00Z

PR Checklist

The PR title follows our guidelines: https://github.com/NativeScript/NativeScript/blob/master/CONTRIBUTING.md#commit-messages.
There is an issue for the bug/feature this PR is for. To avoid wasting your time, it's best to open a suggestion issue first and wait for approval before working on it.
You have signed the CLA.
All existing tests are passing: https://github.com/NativeScript/nativescript-cli/blob/master/CONTRIBUTING.md#contribute-to-the-code-base
Tests for the changes are included.

What is the current behavior?

Limited to only use NPM as package manager.

What is the new behavior?

Add support for the Yarn package manager.

Implements #2737.

We split this into two parts since it was a big diff. Can you please review part 2 ? CC: @rosen-vladimirov

Also do I qualify for the amazon gift card since it's my first contribution? 😛

…er instance

Fatme · 2018-10-23T05:30:14Z

lib/base-package-manager.ts

+export class BasePackageManager {
+	constructor(private packageManager: string) { }
+
+	protected getNpmExecutableName(isWindows: boolean): string {


No need to provide isWindows as param. You can determine it inside the method. Just need to inject $hostInfo.

constructor(private packageManager: string, private $hostInfo: IHostInfo){ }

protected getNpmExecutableName(): string { const isWindows = this.$hostInfo.isWindows; }

You'll need to change this https://github.com/NativeScript/nativescript-cli/pull/4050/files#diff-5d0fca739bb2fbd7cab602dbeaa30b89R17 as well

super('npm', $hostInfo);

This also should be changed https://github.com/NativeScript/nativescript-cli/pull/4050/files#diff-5d0fca739bb2fbd7cab602dbeaa30b89L11

constructor($hostInfo: IHostInfo, ...) { }

NOTE: No private before $hostInfo.

Fatme · 2018-10-23T05:43:23Z

lib/base-package-manager.ts

+
+			const npmExecutable = this.getNpmExecutableName(isWindows);
+
+			if (childProcess.stdout) {


This code is duplicated to this https://github.com/NativeScript/nativescript-cli/blob/master/lib/common/child-process.ts#L57
So I suggest to inject $childProcess: IChildProcess in constructor and reuse the logic here.

protected async processPackageManagerInstall(params: string[], opts: { cwd: string }) { const npmExecutable = this.getNpmExecutableName(); const stdioValue = isInteractive() ? "inherit" : "pipe"; const result = await this.$childProcess.spawnFromEvent(npmExecutable, params, "close", { cwd: opts.cwd, stdio: stdioValue }); return result; }

These 2 methods should be deleted in this case:

https://github.com/NativeScript/nativescript-cli/pull/4050/files#diff-5d0fca739bb2fbd7cab602dbeaa30b89R215

https://github.com/NativeScript/nativescript-cli/pull/4050/files#diff-f2195087b0cc167a75dcffcda9961acaR104

…e class

Fatme

Great work! Thanks for the contribution.

Fatme · 2018-10-24T07:47:48Z

lib/package-manager.ts

+		private $yarn: INodePackageManager,
+		private $userSettingsService: IUserSettingsService
+	) {
+		this._determinePackageManager();


This method is async so it is possible to call some method from the class and this.packageManager to be null.
So I suggest an approach similar to this https://github.com/NativeScript/nativescript-cli/blob/master/lib/common/mobile/android/android-debug-bridge.ts#L56.

@cache() private async init(): Promise<void> { this.packageManager = await this._determinePackageManager(); } @exported("packageManager") @invokeInit() public install(packageName: string, pathToSave: string, config: INodePackageManagerInstallOptions): Promise<INpmInstallResultInfo> { return this.packageManager.install(packageName, pathToSave, config); }

This way this._determinePackageManager(); should be removed from constructor and all methods that relied on this.packageManager should be decorated with @invokeInit()

On the other side init() method is decorated with cache() decorator so the value will be persisted and this._determinePackageManager() will be called only once.

Also we can rewrite _determinePackageManager method:

private async _determinePackageManager(): Promise<void> { let pm = null; try { pm = await this.$userSettingsService.getSettingValue('packageManager'); } catch (err) { this.$errors.fail(`Unable to read package manager config from user settings ${err}`); } if (pm === 'yarn' || this.$options.yarn) { this.packageManager = this.$yarn; } else { this.packageManager = this.$npm; } }

Fatme · 2018-10-24T07:49:29Z

lib/yarn-package-manager.ts

+
+	@exported("yarn")
+	getCachePath(): Promise<string> {
+		throw new Error("Method not implemented.");


this.$errors.fail("Method not implemented.");

Fatme · 2018-10-24T07:49:49Z

lib/yarn-package-manager.ts

+
+	@exported("yarn")
+	public search(filter: string[], config: IDictionary<string | boolean>): Promise<string> {
+		throw new Error("Method not implemented. Yarn does not support searching for packages in the registry.");


this.$errors.fail("Method not implemented. Yarn does not support searching for packages in the registry.");

Fatme · 2018-10-24T18:05:52Z

Also do I qualify for the amazon gift card since it's my first contribution?

Yes, absolutely! Congratulations on this quality PR! Have a badge!

…ds of package manager

dcarrot2 · 2018-10-24T18:57:03Z

@Fatme Thank you for the thorough feedback! Can you pls take a look at Part 2 of this once this looks good to you? We'll merge that in here once it looks good prior to merging this diff in.

mflor35 · 2018-10-24T19:09:07Z

@Fatme Thanks! Our motivation was not the gift card 😅. We actually want to start using yarn to continue the development of The California Court Access App. So far we noticed some speed improvements when building for Android.

Fatme · 2018-10-24T19:24:38Z

@dcarrot2, @mflor35 ,

I reviewed part2 of the PR and it seems ok to me. So you can merge it here.

refactor() - [Yarn Support - Part 2] - Replace npm with package manag…

DimitarTachev

Great Work! 💯

Fatme · 2018-10-25T12:58:42Z

@dcarrot2, @mflor35,

I was ready to merge your PR and noticed the followings:

npmInstallationManager class that is registered in injector here https://github.com/mflor35/nativescript-cli/pull/10/files#diff-91928defa835fe6d9e1ae815324a33f1R87 does not use this.packageManager. It has hardcoded this.$npm.install() and this.$npm.view(). So in case when packageManager is yarn, npm will be used from this class.

I suggest to integrate packageManager inside npmInstallationManager and rename the class to packageInstallationManager.

export class PackageInstallationManager {
    	constructor(private $packageManager: INodePackageManager, ...) { }

     	private async getVersion(packageName: string, version: string): Promise<string> {
		const data: any = await this.$packageManager.view(packageName, { "dist-tags": true });
		this.$logger.trace("Using version %s. ", data[version]);

		return data[version];
	}
}
$injector.register("packageInstallationManager", PackageInstallationManager);

Also all references to npmInstallationManager should be changed to packageInstallationManager.

dcarrot2 · 2018-10-25T23:55:21Z

@Fatme Thanks for catching that! We'll also open another diff after this to refactor the INodePackageManager to IPackageManager and similar changes to make this change clear everywhere else in the codebase.

dcarrot2 · 2018-10-30T18:05:36Z

@Fatme Are we ok to merge this?

Fatme · 2018-10-31T14:10:14Z

@dcarrot2,

Yes, absolutely! I'm waiting for green light from our QAs and after that I'll merge the PR.

radeva · 2018-11-19T14:39:42Z

Hi @mflor35 and @dcarrot2 ,

Congratulations on being two of the winners in the {N} First-time contributors contest!

You can claim your prize by contacting us at nativescriptwinners[at]progress.com not later than Nov 30th 2018 . Make sure you send us the following info:
👉Your full name
👉Your email
👉Your country or residence

Best regards,
The NativeScript Team

msc-ddiaz and others added 4 commits October 22, 2018 15:41

feat() - [Yarn Support - Part 1] Yarn and Package Manager implementation

0f66b4e

refactor() - [Yarn Support - Part 2] - Replace npm with package manag…

666c4d8

…er instance

Fix linting issue

fae38b7

Fix url not being contructed properly

e18fa02

Fatme reviewed Oct 23, 2018

View reviewed changes

msc-ddiaz added 2 commits October 23, 2018 10:13

fix() - use spawnFromEvent to dedup and inject needed services to bas…

6e28e94

…e class

Merge branch 'feat/pkg-mngr-impl' into refactor/pkg-mgr-replace

80f2d64

Fatme reviewed Oct 24, 2018

View reviewed changes

radeva added the ♥ community PR label Oct 24, 2018

msc-ddiaz added 2 commits October 24, 2018 11:15

fix() - ensure settings fs read happens prior to calling member metho…

1364bec

…ds of package manager

Merge branch 'feat/pkg-mngr-impl' into refactor/pkg-mgr-replace

90d6e95

Merge pull request #10 from mflor35/refactor/pkg-mgr-replace

7f14263

refactor() - [Yarn Support - Part 2] - Replace npm with package manag…

mflor35 changed the title ~~feat() - [Yarn Support - Part 1] Yarn and Package Manager implementation~~ feat() - [Yarn Support - Part 1 and 2] Yarn and Package Manager implementation Oct 24, 2018

DimitarTachev approved these changes Oct 25, 2018

View reviewed changes

fix() - ensure packageManager is used everywhere

646aa5d

Fatme approved these changes Nov 1, 2018

View reviewed changes

Fatme merged commit 141dfe2 into NativeScript:master Nov 1, 2018


		const npmExecutable = this.getNpmExecutableName(isWindows);

		if (childProcess.stdout) {

Uh oh!

feat() - [Yarn Support - Part 1 and 2] Yarn and Package Manager implementation #4050

feat() - [Yarn Support - Part 1 and 2] Yarn and Package Manager implementation #4050

Uh oh!

Conversation

mflor35 commented Oct 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Checklist

What is the current behavior?

What is the new behavior?

Uh oh!

Fatme Oct 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fatme Oct 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fatme left a comment

Choose a reason for hiding this comment

Uh oh!

Fatme Oct 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fatme Oct 24, 2018

Choose a reason for hiding this comment

Uh oh!

Fatme Oct 24, 2018

Choose a reason for hiding this comment

Uh oh!

Fatme commented Oct 24, 2018

Uh oh!

dcarrot2 commented Oct 24, 2018

Uh oh!

mflor35 commented Oct 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Fatme commented Oct 24, 2018

Uh oh!

DimitarTachev left a comment

Choose a reason for hiding this comment

Uh oh!

Fatme commented Oct 25, 2018

Uh oh!

dcarrot2 commented Oct 25, 2018

Uh oh!

dcarrot2 commented Oct 30, 2018

Uh oh!

Fatme commented Oct 31, 2018

Uh oh!

radeva commented Nov 19, 2018

Uh oh!

Uh oh!

mflor35 commented Oct 22, 2018 •

edited

Loading

Fatme Oct 23, 2018 •

edited

Loading

Fatme Oct 23, 2018 •

edited

Loading

Fatme Oct 24, 2018 •

edited

Loading

mflor35 commented Oct 24, 2018 •

edited

Loading