Skip to content

Refactor visibility to fix numerous environmental bugs - #2116

Merged
sebmck merged 1 commit into
masterfrom
refactor-visiblity
Dec 3, 2016
Merged

Refactor visibility to fix numerous environmental bugs#2116
sebmck merged 1 commit into
masterfrom
refactor-visiblity

Conversation

@sebmck

@sebmck sebmck commented Dec 2, 2016

Copy link
Copy Markdown
Contributor

Summary

This PR refactors package visibility, simplifies it heavily and fixes numerous bugs around production and optional dependencies.

  • Properly retrieve production flag from config rather than an Install instance.
  • Mark failed optional dependencies as ignored.
  • Filter out dev dependencies in production mode at a later stage rather than intermittently.

Test plan

npm run test

This was referenced Dec 2, 2016
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
@sebmck
sebmck force-pushed the refactor-visiblity branch from b98be45 to 8be51e4 Compare December 2, 2016 11:18
@sebmck sebmck mentioned this pull request Dec 2, 2016
Closed
bestander
bestander reviewed Dec 2, 2016
View reviewed changes

@bestander bestander left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few nits

Comment thread __tests__/commands/install.js Outdated
}, create);
});

test.concurrent('--production flag ignores dev dependencies', () => {

@bestander bestander Dec 2, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we also need a test for #2073 - when a dev dependency is a transitive dependency of a prod dependency.

@torifat torifat Dec 2, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a test for that in #2044

Comment thread src/config.js
key: String(opts.key || this.getOption('key') || ''),
});

if (this.getOption('production') || process.env.NODE_ENV === 'production') {

@bestander bestander Dec 2, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We had a bug recently in React Native.
One of the scripts that calls yarn install had NODE_ENV = production and I could not override this with --no-production.
Should we open a separate issue for that?

@bestander bestander Dec 2, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a PR for that #2057

wyze
wyze reviewed Dec 2, 2016
View reviewed changes
Comment thread src/cli/commands/install.js
Object.assign(manifest, json);

const pushDeps = (depType, {hint, visibility, optional}) => {
const pushDeps = (depType, {hint, optional}, isUsed) => {

@wyze wyze Dec 2, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know your stance/thoughts on default parameters, but you could default isUsed = true here and in the one instance below just pass in the evaluation of production flag.

@sebmck sebmck Dec 2, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've changed the callsites so there's just one use of true now. Eventually if we add a --dev-only flag we'd have to remove it anyway so I don't think there's much value.

@sebmck
sebmck force-pushed the refactor-visiblity branch from 8be51e4 to 0a0f445 Compare December 2, 2016 15:17
@hybrist

hybrist commented Dec 2, 2016

Copy link
Copy Markdown
Contributor

Just tried this against one of our apps and I'm still seeing problems:

  1. yarn --prod doesn't prune all non-prod dependencies. Might be connected to...
  2. yarn --prod installs dependencies of packages it doesn't install.

Example for 2.:

> ll node_modules/testium/
total 0
drwxr-xr-x  5 jkrems  staff   170B Dec  2 09:13 node_modules/

> ll node_modules/testium/node_modules
total 0
drwxr-xr-x  30 jkrems  staff   1.0K Dec  2 09:13 lodash/
drwxr-xr-x   9 jkrems  staff   306B Dec  2 09:13 minimist/
drwxr-xr-x  10 jkrems  staff   340B Dec  2 09:13 mkdirp/

I suspect the problem is that those lodash/minimist/mkdirp are used elsewhere in the tree and the "is in use" logic doesn't handle the case where the same pkg@version might be sometimes in use and sometimes not.

@sebmck

sebmck commented Dec 2, 2016

Copy link
Copy Markdown
Contributor Author

@jkrems Try the latest version of this PR, I've pushed a few times since creating it.

@sebmck
sebmck force-pushed the refactor-visiblity branch from 0a0f445 to 40fe079 Compare December 2, 2016 15:27
@hybrist

hybrist commented Dec 2, 2016

Copy link
Copy Markdown
Contributor

Same result w/ 40fe079.

bestander
bestander reviewed Dec 2, 2016
View reviewed changes
Comment thread __tests__/commands/install/integration.js

const install2 = new Install({flat: true}, config, reporter, lockfile);
expect(install2.generateIntegrityHash('foo', [])).not.toEqual(install.generateIntegrityHash('foo', []));
test.concurrent('hoisting should factor ignored dependencies', async () => {

@bestander bestander Dec 2, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice!

@bestander

bestander commented Dec 2, 2016

Copy link
Copy Markdown
Member

Looks good, do you know why it fails on Windows with this?

production mode with deduped dev dep shouldn't be removed
    SyntaxError: Unexpected token : in JSON at position 13
        at Object.parse (native)

bestander
bestander approved these changes Dec 2, 2016
View reviewed changes

@bestander bestander left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, merge once the Windows build is green

@sebmck

sebmck commented Dec 2, 2016 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@jkrems Can you put together some small steps that I can use to repro? I'm not sure what issue you're describing so it's hard for me to reproduce or add a test for it.

@sebmck
sebmck force-pushed the refactor-visiblity branch 2 times, most recently from 1a1d038 to 2c8d15f Compare December 2, 2016 16:21
@hybrist

hybrist commented Dec 2, 2016

Copy link
Copy Markdown
Contributor

Sorry, I assume I forgot to run npm run build last time I tried (?). For future reference, this was my test case:

rm -rf package.json node_modules && npm init --yes && /path/to/yarn/bin/yarn.js add --dev testium && /path/to/yarn/bin/yarn.js add gofer && /path/to/yarn/bin/yarn.js --prod && ls node_modules/testium*; rm -rf node_modules && /path/to/yarn/bin/yarn.js --prod && ls node_modules/testium*; true

Looks good now! No more non-prod packages in --prod and pruning works as expected.

@coreyc coreyc mentioned this pull request Dec 3, 2016
Closed
@sebmck
sebmck force-pushed the refactor-visiblity branch from 2c8d15f to d15c473 Compare December 3, 2016 14:50
@sebmck

sebmck commented Dec 3, 2016

Copy link
Copy Markdown
Contributor Author

Tests are passing across all CI.

@sebmck
sebmck merged commit 6e7d396 into master Dec 3, 2016
@sebmck
sebmck deleted the refactor-visiblity branch December 3, 2016 14:58
This was referenced Dec 3, 2016
Closed
Closed
@mvestergaard mvestergaard mentioned this pull request Dec 4, 2016
Closed
@juanca juanca mentioned this pull request Mar 17, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bestander bestander bestander approved these changes
+2 more reviewers
@wyze wyze wyze left review comments
@torifat torifat torifat left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@sebmck @hybrist @bestander @wyze @torifat