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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 10 commits into from Nov 1, 2018

Conversation

Projects
None yet
5 participants
@mflor35
Contributor

mflor35 commented Oct 22, 2018

PR Checklist

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? 馃槢

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

This comment has been minimized.

@Fatme

Fatme Oct 23, 2018

Contributor

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.

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

This comment has been minimized.

@Fatme

Fatme Oct 23, 2018

Contributor

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:

  1. https://github.com/NativeScript/nativescript-cli/pull/4050/files#diff-5d0fca739bb2fbd7cab602dbeaa30b89R215
  2. https://github.com/NativeScript/nativescript-cli/pull/4050/files#diff-f2195087b0cc167a75dcffcda9961acaR104
@Fatme

Great work! Thanks for the contribution.

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

This comment has been minimized.

@Fatme

Fatme Oct 24, 2018

Contributor

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;
    }	
}
@exported("yarn")
getCachePath(): Promise<string> {
throw new Error("Method not implemented.");

This comment has been minimized.

@Fatme

Fatme Oct 24, 2018

Contributor
this.$errors.fail("Method not implemented.");
@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 comment has been minimized.

@Fatme

Fatme Oct 24, 2018

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

This comment has been minimized.

Contributor

Fatme commented Oct 24, 2018

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!
badge

@dcarrot2

This comment has been minimized.

Contributor

dcarrot2 commented Oct 24, 2018

@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

This comment has been minimized.

Contributor

mflor35 commented Oct 24, 2018

@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

This comment has been minimized.

Contributor

Fatme commented Oct 24, 2018

@dcarrot2, @mflor35 ,

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

Merge pull request #10 from mflor35/refactor/pkg-mgr-replace
refactor() - [Yarn Support - Part 2] - Replace npm with package manag鈥

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

@DimitarTachev

Great Work! 馃挴

@Fatme

This comment has been minimized.

Contributor

Fatme commented Oct 25, 2018

@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

This comment has been minimized.

Contributor

dcarrot2 commented Oct 25, 2018

@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

This comment has been minimized.

Contributor

dcarrot2 commented Oct 30, 2018

@Fatme Are we ok to merge this?

@Fatme

This comment has been minimized.

Contributor

Fatme commented Oct 31, 2018

@dcarrot2,

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

@Fatme

Fatme approved these changes Nov 1, 2018

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

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@radeva

This comment has been minimized.

Contributor

radeva commented Nov 19, 2018

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

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