Introducing `y-npm` (aka: Why NPM?) #162

RomainMuller · 2018-06-21T14:27:28Z

A drop-in replacement for the npm command that uses a local repository as a source for packages, and falls back to the standard npm registry for packages that are not locally available.

This can be used to make private packages available to consumers without the need to set up a full blown repository and managing user authentication and authorization; and also to overlay customized versions of third party packages in a simple way.

The y-npm command has an additional sub-command y-npm y-ls that will output the name and version of all packages that are locally available.

This commit also replaces cdk-beta-npm in favor of using y-npm. Also, the release .zip file no longer includes a full installation of the cdk, instead only makes y-npm, so people can use y-npm instead of npm and work as they normally would besides that (this reduces the file size of the release zip by about 50%).

eladb

Didn't go through everything yet, see my note about bundling...

eladb · 2018-06-21T15:02:30Z

bundle-beta.sh

+    ${Y_NPM} publish ${tarball}
+done
+
+echo "Installing y-npm" # using my-npm!


"my-npm" => "y-npm"

🤦🏻‍♂️

eladb · 2018-06-21T15:11:52Z

bundle-beta.sh

+# Symlink 'bin' to the root
+echo "Installing cdk-beta-npm"
+mkdir -p bin
+cat > bin/cdk-beta-npm <<-'HERE'


Let's get rid of cdk-beta-npm altogether!

I think the name is wrong in 4 different ways (too long, confusing, ugly, non-ergonomic).

We are going to have y-mvn and y-nuget and eventually we'll publish y-repo as a separate project and can point people to it's docs in case they have any questions or issues. Adding another abstraction layer on top of it will make it hard for people to understand what's going on.

How do we get rid of it? Include most of the bundling logic into y-npm. Think about other users of y-npm - they will effectively have to re-write the code in bundle-beta.sh.

Here's a proposal for an API:

$ y-npm y-stage <tarballs> bin/y-npm node_modules/y-npm node_modules/... y/npm/<verdaccio-tree>

This will allow you to define the default for behavior in case Y_NPM_REPO is not defined to auto-locate it's repo y/npm/ based on where the y-npm module is installed path.join(__dirname, '../../y.npm').

This will also apply exactly in the same way to y-mvn.

Eventually, all one will need to do is zip the staging directory and be happy!

It took me reading your suggestion thrice, but now that I understand it, I like it 👍
I'll use y/npm repository resolution akin to what node does:

${Y_NPM_REPOSITORY}

${cwd}/y/npm

Walk up the tree from ${cwd} until:

A y/npm directory is found

The root of the filesystem is reached

Walk up the tree from path.join(require.resolve('y-npm/package.json'), '..', '..') until:

A y/npm directory is found

The root of the filesystem is reached

Bail out in error.

sounds great!

Now done 👌🏻

eladb

You should write a couple of tests to get at least minimal coverage of the behavior. I would write a few bash scripts that exercise the tool in a few paths.

I am still not in ❤️ with our eventual setup story but we can punt that for a subsequent iteration.

eladb · 2018-06-21T16:41:29Z

CHANGELOG.md

+
+ * Stopped bundling all dependencies in the release ZIP ([@RomainMuller] in [#161] and [awslabs/jsii#43])
+ * Replaced `cdk-beta-npm` with more generic `y-npm` tool ([@RomainMuller] in [#162])
+ * Improved speed of `pack.sh`


Not needed in changelog

All of them? Or some of them?

eladb · 2018-06-21T16:43:21Z

bundle-beta.sh

+#   aws-cdk-${version}.zip
+#   ├─ bin
+#   ├─ docs
+#   ├─ repo


I will change this to y/mvn shortly

I already changed y/npm :)

eladb · 2018-06-21T16:47:19Z

packages/aws-cdk/lib/init.ts

-    if (await execCdkBetaNpm('install') !== 0) {
-        throw new Error('cdk-beta-npm install failed!');
+    const cdkHome = process.env.CDK_HOME ? path.resolve(process.env.CDK_HOME) : path.join(os.homedir(), '.cdk');
+    const cdkBetaNpm = path.join(cdkHome, 'bin', 'y-npm');


rename variable

eladb · 2018-06-21T16:50:22Z

tools/y-npm/README.md

+  ```
+* For a package from your `npm` registry of choice:
+  ```shell
+  TGZ=npm pack package[@version] # Download the package locally


missing $()

🤦🏻‍♂️

eladb · 2018-06-21T16:50:42Z

tools/y-npm/README.md

+  ```shell
+  TGZ=npm pack package[@version] # Download the package locally
+  y-npm publish ${TGZ}           # Publish the downloaded package
+  rm ${TGZ}                      # Clean-up after yourself


eladb · 2018-06-21T16:53:28Z

tools/y-npm/bin/y-npm.ts

+    const storage = await determineStorageDirectory();
+    if (argv.length === 1) {
+        switch (argv[0]) {
+        case 'my-packages':


shouldn't that be y-packagesor better: y-ls

Like y-ls.

eladb · 2018-06-21T16:54:27Z

tools/y-npm/lib/determine-storage-directory.ts

+import { debug } from './logging';
+
+/** The parent directory of this module's location */
+const installRoot = path.resolve(path.join(__dirname, '..', '..'));


why not just __dirname?

I could start by testing two directories that I know upfront will not contain a y/npm directory, but I chose to save the effort.

Fair enough

eladb · 2018-06-21T16:57:40Z

tools/y-npm/lib/with-temporary-file.ts

+        return await callback(file);
+    } finally {
+        await fs.remove(file);
+        await fs.remove(dir);


cant you rm -fr the dir?

I couldn't find a fs API that would achieve this. I can implement a short recursive remove if you want?

fs-extra has one and it's a good package to depend on

Replacing with fs.emptyDir(dir) then fs.remove(dir). Or is there a call that does both?

I believe fs.remove is idempotent

remove should be sufficient

RomainMuller · 2018-06-22T09:39:43Z

I've changed the set-up process a bit -- the bundle_beta.sh script now installs three packages so they are available from ~/.cdk/bin after install: y-npm, aws-cdk and aws-cdk-docs. This way, one should be able to unzip, change PATH, and start using CDK without further due.

Updated the PR description accordingly.

eladb · 2018-06-22T20:56:18Z

Pre-installing aws-cdk and aws-cdk-docs won't work on Windows (because npm won't create the batch files). See #138, which was the initial motivation for this PR...

I think the instructions of "unzip and run y-npm aws-cdk aws-cdk-docs" are perfectly fine for the beta and in a sense are exactly how the CDK will be installed post-beta (just without the unzip portion). Aligning the beta experience with the post-beta experience (and mechanism) has a lot of value.

eladb · 2018-06-22T20:58:12Z

bundle-beta.sh

@@ -37,8 +37,8 @@ for tarball in $(find ${root}/.local-npm -iname '*.tgz') $(find ${root}/pack -in
    ${Y_NPM} publish ${tarball}
 done

-echo "Installing y-npm" # using y-npm!
-${Y_NPM} install --global-style --no-save y-npm
+echo "Installing y-npm, aws-cdk and aws-cdk-docs" # using y-npm!


This is one step to far... We are losing the idiomatic experience y-npm bought us and won't work on Windows (#138).

eladb · 2018-06-22T21:00:10Z

tools/y-npm/test/test.sh

+}
+trap cleanup EXIT
+
+# Now we'll create a hierarchy:


Can you add a few words on the motivation for this directory structure?

The comments on the structure itself attempt to explain the motivation. Can you elaborate on what is not clear enough? I've updated this a little in the upcoming revision.

eladb · 2018-06-22T21:02:10Z

tools/y-npm/test/test.sh

+    if [ "${expected}" == "${actual}" ]; then
+        echo "✅ ${title}"
+    else
+        echo "❌ ${title}"


I've used bats in the past and loved it.

eladb · 2018-06-22T21:03:20Z

tools/y-npm/bin/y-npm.ts

-        case 'my-packages':
-            debug(`Invoking special command: my-packages`);
+        case 'y-ls':
+            debug(`Invoking special command: y-ls`);


instead of "special" maybe "invoking y-npm command: y-ls" (gives people a reference to where this is coming from)

eladb · 2018-06-25T16:55:35Z

tools/y-npm/lib/with-temporary-file.ts

+        return await callback(file);
+    } finally {
+        await fs.remove(file);
+        await fs.remove(dir);


remove should be sufficient

A drop-in replacement for the `npm` command that uses a local repository as a source for packages, and falls back to the standard `npm` registry for packages that are not locally available. This can be used to make private packages available to consumers without the need to set up a full blown repository and managing user authentication and authorization; and also to overlay customized versions of third party packages in a simple way. The `y-npm` command has an additional sub-command `y-npm y-ls` that will output the name and version of all packages that are locally available. This commit also replaces cdk-beta-npm in favor of using y-npm. Also, the release .zip file no longer includes a full installation of the cdk, instead only makes y-npm, so people can use y-npm instead of npm and work as they normally would besides that (this reduces the file size of the release zip by about 50%).

RomainMuller requested review from rix0rrr and eladb June 21, 2018 14:27

RomainMuller force-pushed the rmuller/y-npm branch from 1ff8d6a to 05f6167 Compare June 21, 2018 14:49

RomainMuller changed the title ~~Indtroducing y-npm (aka: Why NPM?)~~ Introducing y-npm (aka: Why NPM?) Jun 21, 2018

eladb reviewed Jun 21, 2018

View reviewed changes

eladb suggested changes Jun 21, 2018

View reviewed changes

eladb reviewed Jun 22, 2018

View reviewed changes

eladb approved these changes Jun 25, 2018

View reviewed changes

RomainMuller closed this Jun 25, 2018

RomainMuller force-pushed the rmuller/y-npm branch from a1e780d to 0eba137 Compare June 25, 2018 17:09

RomainMuller deleted the rmuller/y-npm branch June 25, 2018 17:09

NGL321 added the contribution/core This is a PR that came from AWS. label Sep 27, 2019

Introducing y-npm (aka: Why NPM?) #162

Introducing y-npm (aka: Why NPM?) #162

Conversation

RomainMuller commented Jun 21, 2018 • edited

eladb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eladb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RomainMuller commented Jun 22, 2018 • edited

eladb commented Jun 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Introducing `y-npm` (aka: Why NPM?) #162

Introducing `y-npm` (aka: Why NPM?) #162

RomainMuller commented Jun 21, 2018 •

edited

RomainMuller commented Jun 22, 2018 •

edited