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

Add a node_scope option to node_module targets to support package-scopes #6616

Merged
merged 19 commits into from Oct 15, 2018

Conversation

Projects
None yet
3 participants
@nsaechao
Copy link
Collaborator

nsaechao commented Oct 10, 2018

This is the implementation of #6540.

Problem

Pants source-level dependencies for Node.js targets does not support node-scopes.

Solution

Add a node_scope config to node_module targets and a repo-level node_scope to node-distribution.

Result

The repo-owner may specify a target-level and/or global node-scope that will resolve all Node.js source-level dependencies under the correct node-scope rules.

For example, if you have a target dependency on src/node/pants/custom/web-package:web-package with a package name of web-package, the installed path under the node_modules directory would be */node_modules/web-package.

If a node_scope=pants is defined for src/node/pants/custom/web-package:web-package. Then the installed directory would be */node_modules/@pants/web-package.

Target-level node_scope overrides repo-level node_scope.

Defining scope in package.json

When dep inference is ready, a potential update to this implementation is to be able to infer the node_scope from the name defined in package.json.

@stuhood stuhood requested review from stuhood and baroquebobcat Oct 12, 2018

@stuhood
Copy link
Member

stuhood left a comment

Great, thanks!

@stuhood stuhood added this to the 1.11.x milestone Oct 15, 2018

for package_name, file_path in source_deps.items():
# Package name should always the same as the target name
dep = self._get_target_from_package_name(target, package_name, file_path)

# Apply node-scoping rules if applicable
node_scope = dep.payload.node_scope or node_task.node_distribution.node_scope

This comment has been minimized.

@baroquebobcat

baroquebobcat Oct 15, 2018

Contributor

I'm a little surprised that the node_distribution is pulled from the node task instead of using global_instance() on NodeDistribution for getting the node_scope.

It looks like NodeDistribution can be task scoped. It's a little confusing to me because I think that means that you could have multiple values of the default node_scope under different option scopes.

Another option would be to move this or'd clause to an @property on the node target class and add a subsystem dependency on NodeDistribution to the target.

This comment has been minimized.

@stuhood

stuhood Oct 15, 2018

Member

It looks like NodeDistribution can be task scoped.

Any subsystem can be scoped or unscoped... how it is consumed is convention. But you're right that this should probably be global.

That would be a separate change/issue though, and shouldn't happen here probably.

This comment has been minimized.

@nsaechao

nsaechao Oct 15, 2018

Collaborator

I'll need to look into why we scope NodeDistribution at the task-level, but it does seem more reasonable to have it at the global level. I believe this may stem from some old history to support multiple versions of Node.js within the same repo.

This comment has been minimized.

@baroquebobcat

baroquebobcat Oct 15, 2018

Contributor

Hm. Ok.

This comment has been minimized.

@nsaechao

nsaechao Oct 15, 2018

Collaborator

I can look into this in another PR if you don't mind

This comment has been minimized.

@stuhood

stuhood Oct 15, 2018

Member

Yes please.

Show resolved Hide resolved .../node/src/python/pants/contrib/node/subsystems/resolvers/npm_resolver.py Outdated
Show resolved Hide resolved .../node/src/python/pants/contrib/node/subsystems/resolvers/npm_resolver.py
@baroquebobcat
Copy link
Contributor

baroquebobcat left a comment

Oh, and it looks good to me, modulo comments. The only one I'd like to see addressed before landing is the node scope sourcing one.

@baroquebobcat baroquebobcat merged commit 31f4000 into pantsbuild:master Oct 15, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment