Fix: Remove devDependencies from list output when environment is production #4092

olingern · 2017-08-04T03:51:22Z

Summary

Fixes: #3788.

This PR loads devDependencies from the manifest, re-builds the patterns package structure, and filters packages if the NODE_ENV is production.

Test plan

Added new tests.

BYK · 2017-08-04T12:57:15Z

Related #4095.

BYK · 2017-08-04T12:58:13Z

src/constants.js

+}
+
+export function isProduction(env: Env): boolean {
+  return getEnv(env) === 'production';


You should make 'development and 'production strings constants too ;)

@BYK So, I looked around the project and noticed 'production' is used elsewhere. I'll add the constants, and maybe open a subsequent PR to update the cases where 'production' is referring to NODE_ENV

BYK · 2017-08-04T12:59:15Z

src/cli/commands/list.js

 export async function run(config: Config, reporter: Reporter, flags: Object, args: Array<string>): Promise<void> {
  const lockfile = await Lockfile.fromDirectory(config.lockfileFolder, reporter);
  const install = new Install(flags, config, reporter, lockfile);
-  const {requests: depRequests, patterns, workspaceLayout} = await install.fetchRequestFromCwd();
+
+


These extra blank lines would probably fail lint.

BYK · 2017-08-04T13:01:12Z

src/cli/commands/list.js

  await install.resolver.init(depRequests, {
    isFlat: install.flags.flat,
    isFrozen: install.flags.frozenLockfile,
    workspaceLayout,
  });

+  let pkgPatterns = [];
+  if (isProduction(process.env)) {


I would just use process.env at all times instead of passing this. Also this should probably be a method on config since we also support passing --prod flag. Not sure that flag applies to list but we can make it so?

I thought about that, but it seemed that it would decrease the testability of isProduction

You should be able to mock process.env during tests ;)

Added tests for these. Feels a little weird to store and restore the NODE_ENV instead of injecting, but if there's no reason for other portions of the code to care about process.env, then this makes sense :)

BYK · 2017-08-04T13:02:26Z

src/cli/commands/list.js

+  let pkgPatterns = [];
+  if (isProduction(process.env)) {
+    const devDeps = getDevDeps(manifest);
+    pkgPatterns = patterns.filter(pattern => !devDeps.includes(pattern));


I would convert devDeps to a Set for faster look up. Technically, both can be sets since this is a set subtraction but the Set class doesn't have that method unless you use a third-party library.

Good point. This is probably O(n)^n since filter loops over the entire collection and then includes checks the entire collection for existence there, too.

devDeps being a set could get the filtering down to constant access to see if a package is a dependency, but I don't see how converting patterns to a set would benefit this particular piece of code, since its creation would be linear as well. I could be blindly missing something, though :)

Where n is the length of devDeps and m being the length of patterns it should go as follows:

O(n) for creating the set

O(1) for each look up

O(m) for creating the other set

O(m) for walking over the set

So I think you're right. Just converting one to a set for faster look up should be the best :)

Oh wait, there's one important nuance tho:

If m > n which usually is the case, then converting m to a set and then going over devDeps (O(n)) and removing items from the set should be faster.

Hmm ...

So, let's say a is our dependencies and b is our dev dependencies.

For production, we're looking for the subset, c, which is found by a - b

In both cases where the length of a > b and the length of b > a you would have to create a set of the largest ( or smallest depending on where you want to take the linear hit ) and then iterate over the other collection to find the subset of a - b.

I think converting devDeps to a set will get us to O(n) + m, which is pretty okay :)

BYK · 2017-08-09T16:54:42Z

Well, you broke the build :D

BYK

Please fix the build.

BYK · 2017-08-09T16:53:16Z

__tests__/constants.js

@@ -10,3 +10,45 @@ test('getPathKey', () => {
  expect(getPathKey('linux', {})).toBe('PATH');
  expect(getPathKey('darwin', {})).toBe('PATH');
 });
+
+test('NODE_ENV_DEV', () => expect(NODE_ENV_DEV).toBe('development'));


Don't think these are useful :)

olingern · 2017-08-11T02:59:01Z

@BYK Build fixed. Was going to test stdout. Let me know what you think:

1.) Install packages from a fixture that has a dev dependency
2.) Run in development. Make sure devDependency is in the output
3.) Run in production. Make sure dev dependency is not in the output

BYK · 2017-08-14T12:53:24Z

@olingern that plans sounds good to me. Keep in mind that you can create your own small fixtures using file: protocol so you don't need "real" packages for anything.

BYK

I have a few comments but overall looks good.

I can land this as it is but if you want to go back one last round to address my comments, I'm good with that too. Holding the merge until I hear back from you :)

BYK · 2017-08-14T12:57:43Z

__tests__/commands/list.js

-    const trees = [makeTree('gulp@3.9.1', {color: 'bold'}), makeTree('gulp-babel@6.1.2', {color: 'bold'})];
+  test('lists all dependencies when not production', (): Promise<void> => {
+    const nodeEnv = process.env.NODE_ENV;
+    process.env.NODE_ENV = 'development';


Mocking isProduction may be the better approach here since that should also be affected by the --production flag on the CLI.

BYK · 2017-08-14T12:59:24Z

__tests__/commands/list.js

+
+      rprtr.tree('list', trees);
+
+      expect(tree).toEqual(rprtr.getBuffer());


May be instead of constructing the tree manually, you can just do expect(reporter.getBuffer()).toMatchSnapshot()?

I'm new-er to Jest, and just watched @cpojer's talk on Jest's new features. As I understand it, snapshots don't ensure validity, but keep a historical record (snapshot) of the current state. I could see snapshots being useful for quick smoke tests, but I'm curious to how you would use them here.

Here's my reasoning:

If you manually tested this and know this output is correct, we can safely assume that the output should not change unless the behavior changes. Thus, we can store this sample output as a reference and in this output we should/should not have devDependencies based on the prod state. Makes sense?

If, for some reason, the functionality of how yarn list changed -- .toMatchSnapshot() would catch the error, but it wouldn't give a diff on why / how to fix it (or maybe it would, I'm not privy to snapshot's expected / result output).

but it wouldn't give a diff on why / how to fix it (or maybe it would, I'm not privy to snapshot's expected / result output).

It would :) It'd say "I expected this, and I got this other thing instead. Here's the diff."

BYK · 2017-08-14T13:02:28Z

__tests__/constants.js

@@ -10,3 +10,22 @@ test('getPathKey', () => {
  expect(getPathKey('linux', {})).toBe('PATH');
  expect(getPathKey('darwin', {})).toBe('PATH');
 });
+
+test('isProduction', () => {


Protip: you can have describe('isProduction') at the top that wraps all this and then put each separate scenario in their own test() calls.

Btw. changing globals like process.env.NODE_ENV in tests is a bit dangerous since we run tests in parallel. It may yield to other problems. A no-to-great way is to define isProduction as follows that lets you do dependency injection:

function isProduction(env = process.env) { return env.NODE_ENV === 'production'; }

Yah, wasn't sure of the overall test structure / style. It seemed that mosts tests are at parent level to each file, so I didn't know if nesting tests inside of a describe was good or bad style here.

Btw. changing globals like process.env.NODE_ENV in tests is a bit dangerous since we run tests in parallel

Agreed. Wasn't sure of how to successfully mock process.env.NODE_ENV, but your advice above sorts that out :)

It seemed that mosts tests are at parent level to each file, so I didn't know if nesting tests inside of a describe was good or bad style here.

Yeah, that's something I want to address soon. Apologies for the confusion :(

BYK · 2017-08-14T13:03:15Z

__tests__/fixtures/list/dev-deps-prod/package.json

@@ -0,0 +1,8 @@
+{
+  "dependencies": {
+    "left-pad": "^1.1.3"


Instead of relying on "real" dependencies, you can just construct local packages which may simplify and speed up things.

Hmmm ... could you point me to a test that follows that pattern? I was basing most of my structure off of the no-args tests / fixtures

Try this: https://github.com/yarnpkg/yarn/tree/master/__tests__/fixtures/install/install-file-relative ;)

BYK · 2017-08-14T13:04:28Z

src/cli/commands/list.js

@@ -180,21 +181,36 @@ export function filterTree(tree: Tree, filters: Array<string>): boolean {
  return notDim && (found || hasChildren);
 }

+export function getDevDeps(manifest: Object): Set<string> {
+  const devDepSet = new Set();
+  Object.keys(manifest.devDependencies).forEach(key => devDepSet.add(`${key}@${manifest.devDependencies[key]}`));


Even better:

return new Set(Object.keys(manifest.devDependencies).map(key => `${key}@${manifest.devDependencies[key]}`));

Nice, will amend

BYK · 2017-08-14T13:06:36Z

src/cli/commands/list.js

  await install.resolver.init(depRequests, {
    isFlat: install.flags.flat,
    isFrozen: install.flags.frozenLockfile,
    workspaceLayout,
  });

+  let pkgPatterns = [];


May be a more explanatory name than pkgPatterns? May be usedPatterns or activePatterns?

Good point. activePatterns has a nice "ring" to it.

BYK · 2017-08-14T14:20:05Z

@olingern ping me when you push the final update then!

olingern · 2017-08-14T14:29:56Z

@BYK I have couple questions / comments, but will get around to making all the fixes later today. 🎉

load devDeps from manifest and filter if production add constants for node env and dont pass in env to getEnv add tests for NODE_ENV_DEV, NODE_ENV_PROD, getEnv, and isProduction remove extra line break Test list output dependent upon environment rename pkgPatterns update list test with snapshot and use local pkg

olingern · 2017-08-15T05:43:55Z

@BYK Made most of the updates, but couldn't find a way to mock isProduction via requiring the constants file.

Seems like there may be an answer here, but I'll have to take a look at it tomorrow.

BYK · 2017-08-15T13:46:44Z

Seems like there may be an answer here, but I'll have to take a look at it tomorrow.

That seems like the right place for me :) Seems like you have a few lint issues (yarn lint to see them locally). Fix them and we can merge the patch!

Thanks a lot for all the follow-ups, very much appreciated!

olingern · 2017-08-17T17:51:14Z

@arcanis I can make sure linting / tests pass today

arcanis · 2017-08-17T17:54:21Z

That would be great 😃 I tried running prettier, but unfortunately some Flow errors remain

mock node env

olingern · 2017-08-18T13:10:45Z

@BYK @arcanis linting fixed, tests updated

olingern changed the title ~~Add helper env functions~~ Remove devDependencies from list command when environment is production Aug 4, 2017

olingern changed the title ~~Remove devDependencies from list command when environment is production~~ Remove devDependencies from list output when environment is production Aug 4, 2017

BYK reviewed Aug 4, 2017

View reviewed changes

BYK requested changes Aug 9, 2017

View reviewed changes

BYK approved these changes Aug 14, 2017

View reviewed changes

BYK approved these changes Aug 15, 2017

View reviewed changes

Merge branch 'master' into bug/fix-ls-on-production

ffe7863

Maël Nison and others added 2 commits August 18, 2017 01:14

Fixes linting

fe08e3c

mock node env

Merge branch 'master' into bug/fix-ls-on-production

99ec774

BYK changed the title ~~Remove devDependencies from list output when environment is production~~ Fix: Remove devDependencies from list output when environment is production Aug 18, 2017

BYK merged commit 2952f3c into yarnpkg:master Aug 18, 2017


		rprtr.tree('list', trees);

		expect(tree).toEqual(rprtr.getBuffer());

Fix: Remove devDependencies from list output when environment is production #4092

Fix: Remove devDependencies from list output when environment is production #4092

Conversation

olingern commented Aug 4, 2017 • edited by BYK Loading

BYK commented Aug 4, 2017

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

olingern Aug 5, 2017 • edited Loading

Choose a reason for hiding this comment

BYK commented Aug 9, 2017

BYK left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olingern commented Aug 11, 2017 • edited Loading

BYK commented Aug 14, 2017

BYK 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

olingern Aug 14, 2017 • edited Loading

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

BYK commented Aug 14, 2017

olingern commented Aug 14, 2017

olingern commented Aug 15, 2017

BYK commented Aug 15, 2017

olingern commented Aug 17, 2017

arcanis commented Aug 17, 2017

olingern commented Aug 18, 2017

olingern commented Aug 4, 2017 •

edited by BYK

Loading

olingern Aug 5, 2017 •

edited

Loading

olingern commented Aug 11, 2017 •

edited

Loading

olingern Aug 14, 2017 •

edited

Loading