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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

node_modules and node_test support yarnpkg as package manager #4255

Merged
merged 16 commits into from Feb 27, 2017

Conversation

Projects
None yet
4 participants
@JasonQSong
Contributor

JasonQSong commented Feb 13, 2017

Problem

#4164

Yarnpkg is a new package manager for Node.js community, which can be an alternative to npm. It solves the uncertainty of using npm to solve the dependency graph.

Solution

Read package manager settings from either 'pants.ini:node-distribution:package_manager or node_module target. Using the yarnpkg command if it is set to 'yarn'

Result

@stuhood

Thanks @JasonQSong ! This looks solid.

@@ -0,0 +1 @@
node_modules

This comment has been minimized.

@stuhood

stuhood Feb 15, 2017

Member

When pants runs this target, it will do so under .pants.d... so there shouldn't be a node_modules folder created here.

@stuhood

stuhood Feb 15, 2017

Member

When pants runs this target, it will do so under .pants.d... so there shouldn't be a node_modules folder created here.

This comment has been minimized.

@JasonQSong

JasonQSong Feb 15, 2017

Contributor

I think we still want to allow node developers to develop. If we don't allow node developers to develop in place. If pants is incompatible with all the other tons of dev tools, it's very easy to see that between pants and other tools developers will choose other tools.

@JasonQSong

JasonQSong Feb 15, 2017

Contributor

I think we still want to allow node developers to develop. If we don't allow node developers to develop in place. If pants is incompatible with all the other tons of dev tools, it's very easy to see that between pants and other tools developers will choose other tools.

This comment has been minimized.

@JasonQSong

JasonQSong Feb 16, 2017

Contributor

Actually, if there is no node_modules installed here. It's impossible to generate a yarn.lock file and it is impossible to use yarn as the package manager.

@JasonQSong

JasonQSong Feb 16, 2017

Contributor

Actually, if there is no node_modules installed here. It's impossible to generate a yarn.lock file and it is impossible to use yarn as the package manager.

This comment has been minimized.

@UnrememberMe

UnrememberMe Feb 16, 2017

Contributor

yarn.lock will be checked in and node_modules will/should not be checked in. This means for a freshly installed repo, you will have yarn.lock but no node_modules.

@UnrememberMe

UnrememberMe Feb 16, 2017

Contributor

yarn.lock will be checked in and node_modules will/should not be checked in. This means for a freshly installed repo, you will have yarn.lock but no node_modules.

This comment has been minimized.

@JasonQSong

JasonQSong Feb 16, 2017

Contributor

Yes, that's why node_modules should be in .gitignore

@JasonQSong

JasonQSong Feb 16, 2017

Contributor

Yes, that's why node_modules should be in .gitignore

This comment has been minimized.

@stuhood

stuhood Feb 16, 2017

Member

Ok, can add.

@stuhood

stuhood Feb 16, 2017

Member

Ok, can add.

@@ -0,0 +1,1983 @@
{

This comment has been minimized.

@stuhood

stuhood Feb 15, 2017

Member

Is there a smaller target you could use as an example? Something with fewer deps?

This seems like overkill.

@stuhood

stuhood Feb 15, 2017

Member

Is there a smaller target you could use as an example? Something with fewer deps?

This seems like overkill.

This comment has been minimized.

@JasonQSong

JasonQSong Feb 16, 2017

Contributor

The only dependency I added is the compiler for ES2017, which is a very basic dependency required by most of the node package. It is pretty normal for node package to have this amount of dependencies.

@JasonQSong

JasonQSong Feb 16, 2017

Contributor

The only dependency I added is the compiler for ES2017, which is a very basic dependency required by most of the node package. It is pretty normal for node package to have this amount of dependencies.

This comment has been minimized.

@stuhood

stuhood Feb 16, 2017

Member

Ok, sounds good.

@stuhood

stuhood Feb 16, 2017

Member

Ok, sounds good.

@@ -0,0 +1 @@
node_modules

This comment has been minimized.

@stuhood

stuhood Feb 15, 2017

Member

ditto

@stuhood

stuhood Feb 15, 2017

Member

ditto

Show outdated Hide outdated contrib/node/src/python/pants/contrib/node/subsystems/node_distribution.py
@@ -35,25 +39,40 @@ def register_options(cls, register):
register('--version', advanced=True, default='6.9.1',
help='Node distribution version. Used as part of the path to lookup the '
'distribution with --binary-util-baseurls and --pants-bootstrapdir')
register('--package-manager', advanced=True, default='npm',
help='Default package manager config for repo. Should be one of [npm,yarnpkg]')

This comment has been minimized.

@stuhood

stuhood Feb 15, 2017

Member

register(..) supports a choices arg: choices=['npm', 'yarnpkg'].

Also, you should probably pass fingerprint=True for all of these options.

@stuhood

stuhood Feb 15, 2017

Member

register(..) supports a choices arg: choices=['npm', 'yarnpkg'].

Also, you should probably pass fingerprint=True for all of these options.

Show outdated Hide outdated contrib/node/src/python/pants/contrib/node/subsystems/node_distribution.py
def create(self):
# NB: create is an instance method to allow the user to choose global or scoped.
# It's not unreasonable to imagine multiple Node versions in play; for example: when
# transitioning from the 0.10.x series to the 0.12.x series.
binary_util = BinaryUtil.Factory.create()
options = self.get_options()
return NodeDistribution(binary_util, options.supportdir, options.version)
# for o in options:
# logger.debug('NodeDistrybution.Factory.create options[%s]: %s', o, options[o])

This comment has been minimized.

@stuhood

stuhood Feb 15, 2017

Member

xx

@stuhood
Show outdated Hide outdated contrib/node/src/python/pants/contrib/node/tasks/node_task.py
@@ -17,13 +19,17 @@
from pants.contrib.node.targets.node_test import NodeTest
logger = logging.getLogger(__name__)

This comment has been minimized.

@stuhood

stuhood Feb 15, 2017

Member

Tasks should use self.context.log.debug(..), which puts their log messages in the right scope. Should delete this.

@stuhood

stuhood Feb 15, 2017

Member

Tasks should use self.context.log.debug(..), which puts their log messages in the right scope. Should delete this.

Show outdated Hide outdated contrib/node/src/python/pants/contrib/node/tasks/node_task.py
class NodeTask(Task):
@classmethod
def subsystem_dependencies(cls):
return super(NodeTask, cls).subsystem_dependencies() + (NodeDistribution.Factory,)
@memoized_property
@property
@memoized_method

This comment has been minimized.

@stuhood

stuhood Feb 15, 2017

Member

This is confusing... should switch back to memoized_property.

@stuhood

stuhood Feb 15, 2017

Member

This is confusing... should switch back to memoized_property.

This comment has been minimized.

@JasonQSong

JasonQSong Feb 16, 2017

Contributor

On my own machine, memorized_property will be marked as grammar error by pylint because it is not treated as a property. Does your pylint raise this error or did you abandon this rule?

@JasonQSong

JasonQSong Feb 16, 2017

Contributor

On my own machine, memorized_property will be marked as grammar error by pylint because it is not treated as a property. Does your pylint raise this error or did you abandon this rule?

This comment has been minimized.

@stuhood

stuhood Feb 16, 2017

Member

Pants has its own built in linting, which doesn't trigger for that issue, as far as I can tell.

@stuhood

stuhood Feb 16, 2017

Member

Pants has its own built in linting, which doesn't trigger for that issue, as far as I can tell.

This comment has been minimized.

@JasonQSong

JasonQSong Feb 17, 2017

Contributor

[pylint] E1101:Method 'node_distribution' has no 'package_manager' member
Should I turn off hot grammar check when developing pants?

@JasonQSong

JasonQSong Feb 17, 2017

Contributor

[pylint] E1101:Method 'node_distribution' has no 'package_manager' member
Should I turn off hot grammar check when developing pants?

Show outdated Hide outdated contrib/node/src/python/pants/contrib/node/tasks/node_task.py
"""Returns package manager string for target argument or global config."""
package_manager = target.payload.get_field('package_manager').value
package_manager = package_manager or self.node_distribution.package_manager
logger.debug('NodeTask package manager:%s', package_manager)

This comment has been minimized.

@stuhood

stuhood Feb 15, 2017

Member

This message doesn't include the target, so it isn't super useful. IMO, remove.

@stuhood

stuhood Feb 15, 2017

Member

This message doesn't include the target, so it isn't super useful. IMO, remove.

Show outdated Hide outdated contrib/node/src/python/pants/contrib/node/tasks/node_task.py
package_manager = target.payload.get_field('package_manager').value
package_manager = package_manager or self.node_distribution.package_manager
logger.debug('NodeTask package manager:%s', package_manager)
if package_manager not in ['npm', 'yarnpkg']:

This comment has been minimized.

@stuhood

stuhood Feb 15, 2017

Member

Ditto using a singleton list of choices on NodeDistribution.

@stuhood

stuhood Feb 15, 2017

Member

Ditto using a singleton list of choices on NodeDistribution.

Show outdated Hide outdated contrib/node/src/python/pants/contrib/node/tasks/node_test.py
if result != 0:
raise TaskError('npm test script failed:\n'
'\t{} failed with exit code {}'.format(npm_test_command, result))
logger.debug('First dependency: %s', target.dependencies[0])

This comment has been minimized.

@stuhood

stuhood Feb 15, 2017

Member

...?

@stuhood

This comment has been minimized.

@JasonQSong

JasonQSong Feb 16, 2017

Contributor

This implementation looks super incompatible with pants because node package only has one package.json stay in the package folder. So we simply picked the first dependency as the package to be tested, the second will be dropped. If you have two node packages to be tested. You have to write two separate testing target for them.

@JasonQSong

JasonQSong Feb 16, 2017

Contributor

This implementation looks super incompatible with pants because node package only has one package.json stay in the package folder. So we simply picked the first dependency as the package to be tested, the second will be dropped. If you have two node packages to be tested. You have to write two separate testing target for them.

This comment has been minimized.

@UnrememberMe

UnrememberMe Feb 16, 2017

Contributor
  1. Should be a separate issue / PR. Maybe we should enforce there is only one dependency for node_test
  2. Yes, indeed you will need to have a separate node_test target because the script-name is passed in and can be different for each node_module.
@UnrememberMe

UnrememberMe Feb 16, 2017

Contributor
  1. Should be a separate issue / PR. Maybe we should enforce there is only one dependency for node_test
  2. Yes, indeed you will need to have a separate node_test target because the script-name is passed in and can be different for each node_module.

This comment has been minimized.

@JasonQSong

JasonQSong Feb 17, 2017

Contributor

I didn't add more functionality in this pr. I just add some logs and support for yarnpkg.

@JasonQSong

JasonQSong Feb 17, 2017

Contributor

I didn't add more functionality in this pr. I just add some logs and support for yarnpkg.

Show outdated Hide outdated contrib/node/src/python/pants/contrib/node/subsystems/node_distribution.py
'bin/yarnpkg', self.yarnpkg_version, 'yarnpkg.tar.gz')
logger.debug('Yarnpkg tarball: %s', yarnpkg_tar)
yarnpkg_work_dir = os.path.dirname(yarnpkg_tar)
yarnpkg_umpacked_dir = os.path.join(yarnpkg_work_dir, 'unpacked')

This comment has been minimized.

@mateor

mateor Feb 15, 2017

Member

umpacked typo

@mateor

mateor Feb 15, 2017

Member

umpacked typo

@mateor

I have some coworkers that will be very intrigued by this!

This adds a good bit of code duplication that forks based on package manager details. Perhaps consider adding properties to NodeDistribution or even a NodePackageManager object. They could hold references to name, version, path, and lockfile.

Show outdated Hide outdated contrib/node/examples/src/node/hello/package.json
"main": "index.js",
"repository": "https://github.com/pantsbuild/pants.git",
"author": "pantsbuild",
"license": "MIT",

This comment has been minimized.

@mateor

mateor Feb 16, 2017

Member

I like the MIT license myself. But everything else in pants is Apache 2.0.

@mateor

mateor Feb 16, 2017

Member

I like the MIT license myself. But everything else in pants is Apache 2.0.

This comment has been minimized.

@stuhood

stuhood Feb 16, 2017

Member

+1

@stuhood
@@ -0,0 +1 @@
node_modules

This comment has been minimized.

@UnrememberMe

UnrememberMe Feb 16, 2017

Contributor

yarn.lock will be checked in and node_modules will/should not be checked in. This means for a freshly installed repo, you will have yarn.lock but no node_modules.

@UnrememberMe

UnrememberMe Feb 16, 2017

Contributor

yarn.lock will be checked in and node_modules will/should not be checked in. This means for a freshly installed repo, you will have yarn.lock but no node_modules.

@@ -0,0 +1,19 @@
{

This comment has been minimized.

@UnrememberMe

UnrememberMe Feb 16, 2017

Contributor

do we need npm-shrinkwrap.json for hello-test? If this test is intended to run with Yarn, npm-shrinkwrap.json will not be used.

@UnrememberMe

UnrememberMe Feb 16, 2017

Contributor

do we need npm-shrinkwrap.json for hello-test? If this test is intended to run with Yarn, npm-shrinkwrap.json will not be used.

This comment has been minimized.

@JasonQSong

JasonQSong Feb 16, 2017

Contributor

The example itself shouldn't stick with yarn.

@JasonQSong

JasonQSong Feb 16, 2017

Contributor

The example itself shouldn't stick with yarn.

@@ -0,0 +1,5 @@
node_module(
name='pantsbuild-hello-node',
sources=globs('package.json', 'yarn.lock', 'npm-shrinkwrap.json', '.babelrc', 'index.js'),

This comment has been minimized.

@UnrememberMe

UnrememberMe Feb 16, 2017

Contributor

is npm-shrinkwrap.json necessary here? It seems to be confusing to have both yarn.lock and npm-shrinkwrap.json.

@UnrememberMe

UnrememberMe Feb 16, 2017

Contributor

is npm-shrinkwrap.json necessary here? It seems to be confusing to have both yarn.lock and npm-shrinkwrap.json.

Show outdated Hide outdated contrib/node/src/python/pants/contrib/node/subsystems/node_distribution.py
self._binary_util = binary_util
self._relpath = relpath
self._version = self._normalize_version(version)
package_manager = 'yarnpkg' if package_manager == 'yarn' else package_manager

This comment has been minimized.

@UnrememberMe

UnrememberMe Feb 16, 2017

Contributor

Why not just allow yarnpkg not yarn? This line seems to be unnecessary.

@UnrememberMe

UnrememberMe Feb 16, 2017

Contributor

Why not just allow yarnpkg not yarn? This line seems to be unnecessary.

Show outdated Hide outdated contrib/node/src/python/pants/contrib/node/subsystems/node_distribution.py
TGZ.extract(node_distribution, tmp_dist)
os.rename(tmp_dist, outdir)
return os.path.join(outdir, 'node')
node_tar = self._binary_util.select_binary(self._relpath, self.version, 'node.tar.gz')

This comment has been minimized.

@UnrememberMe

UnrememberMe Feb 16, 2017

Contributor

I am not sure why you changed all local variable names. I am assuming it is done in order to be consistent with yarnpkg_path? The codes here and yarnpkg_path looks very similar, can we potentially consolidate into a library?

@UnrememberMe

UnrememberMe Feb 16, 2017

Contributor

I am not sure why you changed all local variable names. I am assuming it is done in order to be consistent with yarnpkg_path? The codes here and yarnpkg_path looks very similar, can we potentially consolidate into a library?

Show outdated Hide outdated contrib/node/src/python/pants/contrib/node/targets/node_module.py
@@ -24,10 +30,15 @@ def __init__(self, sources=None, address=None, payload=None, **kwargs):
# tasks. The reality is likely to be though that both pants will never cover all cases, and a
# back door to execute new tools during development will be desirable and supporting conversion
# of pre-existing package.json files as node_module targets will require this.
package_manager = 'yarnpkg' if package_manager == 'yarn' else package_manager

This comment has been minimized.

@UnrememberMe

UnrememberMe Feb 16, 2017

Contributor

I suggest we stick withyarnpkg only.

@UnrememberMe

UnrememberMe Feb 16, 2017

Contributor

I suggest we stick withyarnpkg only.

Show outdated Hide outdated contrib/node/src/python/pants/contrib/node/tasks/node_repl.py
# Repl task doesn't make any sence for Node.js with yarnpkg for the following reasons:
# 1. Node.js can simply start from the source root, make an alias for 'cd package_root && node'
# does not give any benefits but increase the maintance work
# 2. There's no simple entry point (binary) for Node.js packages. A package may start with node,

This comment has been minimized.

@UnrememberMe

UnrememberMe Feb 16, 2017

Contributor

I don't see there is anything special about yarnpkg (vs npm) for the repl task. The points you listed above are identical with npm. If the argument is that we shouldn't support pants repl at all for node targets, it should be a separate PR in my POV.

@UnrememberMe

UnrememberMe Feb 16, 2017

Contributor

I don't see there is anything special about yarnpkg (vs npm) for the repl task. The points you listed above are identical with npm. If the argument is that we shouldn't support pants repl at all for node targets, it should be a separate PR in my POV.

@@ -84,6 +99,22 @@ def execute_npm(self, args, workunit_name=None, workunit_labels=None):
workunit_name=workunit_name,
workunit_labels=workunit_labels)
def execute_yarnpkg(self, args, workunit_name=None, workunit_labels=None):

This comment has been minimized.

@UnrememberMe

UnrememberMe Feb 16, 2017

Contributor

The codes will be cleaner if we have a function execute to wrap execute_npm and execute_yarnpkg. This way, all conditional codes between npm and yarnpkg can be encapsulated within the execute method.

@UnrememberMe

UnrememberMe Feb 16, 2017

Contributor

The codes will be cleaner if we have a function execute to wrap execute_npm and execute_yarnpkg. This way, all conditional codes between npm and yarnpkg can be encapsulated within the execute method.

This comment has been minimized.

@JasonQSong

JasonQSong Feb 17, 2017

Contributor

That means we need have a generalized flag for the intention of the command. It seems to be an overkill and doesn't give any benefits and restrict the ability of the execute function because for the upper function invoking execute_package_manager(intention.INSTALL). The function doesn't know what real command is running and losing all the control of what package manager can do unless we provide such intention.

@JasonQSong

JasonQSong Feb 17, 2017

Contributor

That means we need have a generalized flag for the intention of the command. It seems to be an overkill and doesn't give any benefits and restrict the ability of the execute function because for the upper function invoking execute_package_manager(intention.INSTALL). The function doesn't know what real command is running and losing all the control of what package manager can do unless we provide such intention.

Show outdated Hide outdated contrib/node/src/python/pants/contrib/node/tasks/node_test.py
if result != 0:
raise TaskError('npm test script failed:\n'
'\t{} failed with exit code {}'.format(npm_test_command, result))
logger.debug('First dependency: %s', target.dependencies[0])

This comment has been minimized.

@UnrememberMe

UnrememberMe Feb 16, 2017

Contributor
  1. Should be a separate issue / PR. Maybe we should enforce there is only one dependency for node_test
  2. Yes, indeed you will need to have a separate node_test target because the script-name is passed in and can be different for each node_module.
@UnrememberMe

UnrememberMe Feb 16, 2017

Contributor
  1. Should be a separate issue / PR. Maybe we should enforce there is only one dependency for node_test
  2. Yes, indeed you will need to have a separate node_test target because the script-name is passed in and can be different for each node_module.
with open(proof) as fp:
self.assertEqual('42', fp.read().strip())
def test_execute_yarnpkg(self):

This comment has been minimized.

@UnrememberMe

UnrememberMe Feb 16, 2017

Contributor

test_execute_yarnpkg and test_execute_npm can share a large part of the codes.

@UnrememberMe

UnrememberMe Feb 16, 2017

Contributor

test_execute_yarnpkg and test_execute_npm can share a large part of the codes.

This comment has been minimized.

@JasonQSong

JasonQSong Feb 16, 2017

Contributor

Why sharing code across tests

@JasonQSong

JasonQSong Feb 16, 2017

Contributor

Why sharing code across tests

@stuhood

Thanks. I think the question of whether to extract a NodePackageManager class is the last big thing here. Will let you work that out with @UnrememberMe .

Show outdated Hide outdated contrib/node/src/python/pants/contrib/node/subsystems/node_distribution.py
@@ -35,25 +40,46 @@ def register_options(cls, register):
register('--version', advanced=True, default='6.9.1',
help='Node distribution version. Used as part of the path to lookup the '
'distribution with --binary-util-baseurls and --pants-bootstrapdir')
register('--package-manager', advanced=True, default='npm', fingerprint=True,
choices=NodeDistribution.VALID_PACKAGE_MANAGER_LIST.keys(),
help='Default package manager config for repo. Should be one of [npm,yarnpkg]')

This comment has been minimized.

@stuhood

stuhood Feb 20, 2017

Member

[npm,yarnpkg] is inaccurate... could .format() the list in here?

@stuhood

stuhood Feb 20, 2017

Member

[npm,yarnpkg] is inaccurate... could .format() the list in here?

This comment has been minimized.

@JasonQSong

JasonQSong Feb 22, 2017

Contributor

Yes, but, I can't find the help message in ./pants help-all. Should I register this subsystem somewhere?

@JasonQSong

JasonQSong Feb 22, 2017

Contributor

Yes, but, I can't find the help message in ./pants help-all. Should I register this subsystem somewhere?

This comment has been minimized.

@stuhood

stuhood Feb 22, 2017

Member

The option is marked advanced, so it doesn't show in the CLI help unless you pass ./pants help-all --advanced, or use ./pants options (the latter can be easier to grok).

@stuhood

stuhood Feb 22, 2017

Member

The option is marked advanced, so it doesn't show in the CLI help unless you pass ./pants help-all --advanced, or use ./pants options (the latter can be easier to grok).

Show outdated Hide outdated .../node/src/python/pants/contrib/node/subsystems/resolvers/npm_resolver.py
.format(target.address.reference(), npm_install, result))
# Comment out this checkout point because it cannot pass unit tests
# if not os.path.exists('package.json'):
# raise TaskError(

This comment has been minimized.

@stuhood

stuhood Feb 20, 2017

Member

You should be able to. You'll just need to figure out which environment you're running in, and include the relevant files in the workspace.

@stuhood

stuhood Feb 20, 2017

Member

You should be able to. You'll just need to figure out which environment you're running in, and include the relevant files in the workspace.

Show outdated Hide outdated .../node/src/python/pants/contrib/node/subsystems/resolvers/npm_resolver.py
logger.info('Found npm-shrinkwrap.json, do not inject package.json')
else:
logger.warning(
'Cannot find npm-shrinkwrap.json. Did you forget to put it in target sources? '

This comment has been minimized.

@stuhood

stuhood Feb 20, 2017

Member

I like this warning, but it is a little bit long. Maybe drop the "Do you intend to using this experimental functionality?" at the end, as the question is already implied.

@stuhood

stuhood Feb 20, 2017

Member

I like this warning, but it is a little bit long. Maybe drop the "Do you intend to using this experimental functionality?" at the end, as the question is already implied.

Show outdated Hide outdated .../node/src/python/pants/contrib/node/subsystems/resolvers/npm_resolver.py
package_manager = node_task.get_package_manager_for_target(target=target)
if package_manager == 'npm':
if os.path.exists('npm-shrinkwrap.json'):
logger.info('Found npm-shrinkwrap.json, do not inject package.json')

This comment has been minimized.

@stuhood

stuhood Feb 20, 2017

Member

s/do not/will not/

(otherwise it sounds like you're telling the user to do something, rather than telling them that pants is doing it for them)

@stuhood

stuhood Feb 20, 2017

Member

s/do not/will not/

(otherwise it sounds like you're telling the user to do something, rather than telling them that pants is doing it for them)

Show outdated Hide outdated .../node/src/python/pants/contrib/node/subsystems/resolvers/npm_resolver.py
@@ -69,6 +98,7 @@ def _emit_package_descriptor(self, node_task, target, results_dir, node_paths):
dependenciesToRemove = [
'dependencies', 'devDependencies', 'peerDependencies', 'optionalDependencies'
]
logger.error('Removing dependencies')

This comment has been minimized.

@stuhood

stuhood Feb 20, 2017

Member

I don't think people are going to understand this error. Would recommend either removing it, or explicitly saying which dependency is being dropped.

@stuhood

stuhood Feb 20, 2017

Member

I don't think people are going to understand this error. Would recommend either removing it, or explicitly saying which dependency is being dropped.

This comment has been minimized.

@JasonQSong

JasonQSong Feb 22, 2017

Contributor

I was spending over half a day before figuring out why the source file I specified in BUILD is different from the sources in tmp dir. So I believe this logging is important.

@JasonQSong

JasonQSong Feb 22, 2017

Contributor

I was spending over half a day before figuring out why the source file I specified in BUILD is different from the sources in tmp dir. So I believe this logging is important.

This comment has been minimized.

@stuhood

stuhood Feb 22, 2017

Member

That's fine... just make the message more useful by saying what is being removed.

@stuhood

stuhood Feb 22, 2017

Member

That's fine... just make the message more useful by saying what is being removed.

Show outdated Hide outdated contrib/node/src/python/pants/contrib/node/tasks/node_repl.py
@@ -70,3 +70,10 @@ def launch_repl(self, targets):
repl_session = node_repl.run()
repl_session.wait()
# Repl task doesn't make any sence for Node.js with yarnpkg for the following reasons:

This comment has been minimized.

@stuhood

stuhood Feb 20, 2017

Member

Would you mind opening a github issue with this information, and then leaving a shorter TODO/comment here that links to it?

@stuhood

stuhood Feb 20, 2017

Member

Would you mind opening a github issue with this information, and then leaving a shorter TODO/comment here that links to it?

Show outdated Hide outdated contrib/node/src/python/pants/contrib/node/tasks/node_run.py
raise TaskError('npm run script failed:\n'
'\t{} failed with exit code {}'.format(npm_run, result))
package_manager = self.get_package_manager_for_target(target=target)
if package_manager == 'npm':

This comment has been minimized.

@stuhood

stuhood Feb 20, 2017

Member

I agree with Mateo that having all these conditionals spread around is a bit error prone.

It would likely be a good idea to extract an abstract class to represent node package managers, with a method for each of these conditionals. If @UnrememberMe has time to mentor you through that, we should probably do it before landing this review... otherwise, I won't block for it.

@stuhood

stuhood Feb 20, 2017

Member

I agree with Mateo that having all these conditionals spread around is a bit error prone.

It would likely be a good idea to extract an abstract class to represent node package managers, with a method for each of these conditionals. If @UnrememberMe has time to mentor you through that, we should probably do it before landing this review... otherwise, I won't block for it.

This comment has been minimized.

@JasonQSong

JasonQSong Feb 22, 2017

Contributor

It seems like an overkill to extract an abstract class for a simple string comparison. I will extract a const list to reduce the concern of error prone.

@JasonQSong

JasonQSong Feb 22, 2017

Contributor

It seems like an overkill to extract an abstract class for a simple string comparison. I will extract a const list to reduce the concern of error prone.

Show outdated Hide outdated contrib/node/src/python/pants/contrib/node/tasks/node_test.py
@@ -15,6 +17,9 @@
from pants.contrib.node.tasks.node_task import NodeTask
logger = logging.getLogger(__name__)

This comment has been minimized.

@stuhood

stuhood Feb 20, 2017

Member

See previous comment about using a Task's own logger, rather than a static logger.

@stuhood

stuhood Feb 20, 2017

Member

See previous comment about using a Task's own logger, rather than a static logger.

@stuhood

Thanks Jason.

@UnrememberMe

This comment has been minimized.

Show comment
Hide comment
@UnrememberMe

UnrememberMe Feb 27, 2017

Contributor

I am ok with the current codes as it is written. We can refactor out npm/yarn executors in a later time if necessary @JasonQSong @stuhood

Contributor

UnrememberMe commented Feb 27, 2017

I am ok with the current codes as it is written. We can refactor out npm/yarn executors in a later time if necessary @JasonQSong @stuhood

@stuhood stuhood merged commit 78a0daa into pantsbuild:master Feb 27, 2017

1 check passed

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

lenucksi added a commit to lenucksi/pants that referenced this pull request Apr 25, 2017

node_modules and node_test support yarnpkg as package manager (#4255)
### Problem

Yarnpkg is a new package manager for Node.js community, which can be an alternative to npm. It solves the uncertainty of using npm to solve the dependency graph. Pants did not yet support Yarn.

### Solution

Read package manager settings from either `--node-distribution-package-manager` or `node_module` target. Use the yarnpkg command if it is set to 'yarn'.

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