Skip to content
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

feat(typescript): add JSNamedModuleInfo provider to ts_library outputs #1215

Merged
merged 1 commit into from
Oct 9, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 4 additions & 5 deletions WORKSPACE
Original file line number Diff line number Diff line change
Expand Up @@ -67,11 +67,10 @@ yarn_install(
name = "npm",
# The @npm//:node_modules_filegroup generated by manual_build_file_contents
# is used in the //packages/typescript/test/reference_types_directive:tsconfig_types
# test. For now we're still supporting node_modules as a filegroup tho this
# may change in the future. The default generated //:node_modules target is a
# node_module_library rule which provides NodeModuleInfo and NodeModuleSources
# but that rule is not yet in the public API and we have not yet dropped support
# for filegroup based node_modules target.
# test. For now we're still supporting node_modules as a filegroup tho this may
# change in the future. The default generated //:node_modules target is a node_module_library
# rule which provides NpmPackageInfo but that rule is not yet in the public API and we
# have not yet dropped support for filegroup based node_modules target.
manual_build_file_contents = """
filegroup(
name = "node_modules_filegroup",
Expand Down
129 changes: 113 additions & 16 deletions examples/angular/patches/@angular+bazel+9.0.0-next.8.patch
Original file line number Diff line number Diff line change
@@ -1,16 +1,30 @@
diff --git a/node_modules/@angular/bazel/src/esm5.bzl b/node_modules/@angular/bazel/src/esm5.bzl
index 667f601..7907c12 100755
--- a/node_modules/@angular/bazel/src/esm5.bzl
+++ b/node_modules/@angular/bazel/src/esm5.bzl
@@ -30,7 +30,7 @@ ESM5Info = provider(
diff --git a/node_modules/@angular/bazel/src/external.bzl b/node_modules/@angular/bazel/src/external.bzl
index 4959f22..43f1c96 100755
--- a/node_modules/@angular/bazel/src/external.bzl
+++ b/node_modules/@angular/bazel/src/external.bzl
@@ -14,17 +14,17 @@ load(
_ts_providers_dict_to_struct = "ts_providers_dict_to_struct",
)
load(
- "@build_bazel_rules_nodejs//internal/common:node_module_info.bzl",
- _NodeModuleSources = "NodeModuleSources",
- _collect_node_modules_aspect = "collect_node_modules_aspect",
+ "@build_bazel_rules_nodejs//internal/common:npm_package_info.bzl",
+ _NpmPackageInfo = "NpmPackageInfo",
+ _node_modules_aspect = "node_modules_aspect",
)
load(
"@npm_bazel_typescript//internal:ts_config.bzl",
_TsConfigInfo = "TsConfigInfo",
)

def _map_closure_path(file):
- result = file.short_path[:-len(".closure.js")]
+ result = file.short_path[:-len(".mjs")]
-NodeModuleSources = _NodeModuleSources
-collect_node_modules_aspect = _collect_node_modules_aspect
+NpmPackageInfo = _NpmPackageInfo
+node_modules_aspect = _node_modules_aspect

# short_path is meant to be used when accessing runfiles in a binary, where
# the CWD is inside the current repo. Therefore files in external repo have a
tsc_wrapped_tsconfig = _tsc_wrapped_tsconfig
COMMON_ATTRIBUTES = _COMMON_ATTRIBUTES
diff --git a/node_modules/@angular/bazel/src/modify_tsconfig.js b/node_modules/@angular/bazel/src/modify_tsconfig.js
index 9d3c491..3c99746 100755
--- a/node_modules/@angular/bazel/src/modify_tsconfig.js
Expand All @@ -25,14 +39,23 @@ index 9d3c491..3c99746 100755
fs.writeFileSync(output, JSON.stringify(data));
}
diff --git a/node_modules/@angular/bazel/src/ng_module.bzl b/node_modules/@angular/bazel/src/ng_module.bzl
index 9b88fbb..68217d0 100755
index 9b88fbb..54bf2c7 100755
--- a/node_modules/@angular/bazel/src/ng_module.bzl
+++ b/node_modules/@angular/bazel/src/ng_module.bzl
@@ -20,6 +20,7 @@ load(
@@ -13,13 +13,14 @@ load(
"DEFAULT_NG_COMPILER",
"DEFAULT_NG_XI18N",
"DEPS_ASPECTS",
- "NodeModuleSources",
+ "NpmPackageInfo",
"TsConfigInfo",
- "collect_node_modules_aspect",
+ "node_modules_aspect",
"compile_ts",
"ts_providers_dict_to_struct",
"tsc_wrapped_tsconfig",
)
+load("@build_bazel_rules_nodejs//:providers.bzl", "transitive_js_ecma_script_module_info")
+load("@build_bazel_rules_nodejs//:providers.bzl", "js_ecma_script_module_info", "js_named_module_info")
gregmagolan marked this conversation as resolved.
Show resolved Hide resolved

_FLAT_DTS_FILE_SUFFIX = ".bundle.d.ts"
_R3_SYMBOLS_DTS_FILE = "src/r3_symbols.d.ts"
Expand All @@ -54,22 +77,96 @@ index 9b88fbb..68217d0 100755
bundle_index_typings = ctx.actions.declare_file("%s.d.ts" % flat_module_out)
declaration_files.append(bundle_index_typings)
if is_legacy_ngc:
@@ -603,7 +604,17 @@ def ng_module_impl(ctx, ts_compile_actions):
@@ -517,11 +518,11 @@ def _compile_action(ctx, inputs, outputs, dts_bundles_out, messages_out, tsconfi
file_inputs += ctx.attr.tsconfig[TsConfigInfo].deps

# Also include files from npm fine grained deps as action_inputs.
- # These deps are identified by the NodeModuleSources provider.
+ # These deps are identified by the NpmPackageInfo provider.
for d in ctx.attr.deps:
- if NodeModuleSources in d:
+ if NpmPackageInfo in d:
# Note: we can't avoid calling .to_list() on sources
- file_inputs.extend(_filter_ts_inputs(d[NodeModuleSources].sources.to_list()))
+ file_inputs.extend(_filter_ts_inputs(d[NpmPackageInfo].sources.to_list()))

# Collect the inputs and summary files from our deps
action_inputs = depset(
@@ -603,9 +604,23 @@ def ng_module_impl(ctx, ts_compile_actions):
return providers

def _ng_module_impl(ctx):
- return ts_providers_dict_to_struct(ng_module_impl(ctx, compile_ts))
-
-local_deps_aspects = [collect_node_modules_aspect, _collect_summaries_aspect]
+ ts_providers = ng_module_impl(ctx, compile_ts)
+
+ # Add in new JS providers
+ ts_providers["providers"].extend([
+ transitive_js_ecma_script_module_info(
+ js_named_module_info(
+ sources = ts_providers["typescript"]["es5_sources"],
+ deps = ctx.attr.deps,
+ ),
+ js_ecma_script_module_info(
+ sources = ts_providers["typescript"]["es6_sources"],
+ deps = ctx.attr.deps,
+ ),
+ ])
+
+ return ts_providers_dict_to_struct(ts_providers)
+
+local_deps_aspects = [node_modules_aspect, _collect_summaries_aspect]

# Workaround skydoc bug which assumes DEPS_ASPECTS is a str type
[local_deps_aspects.append(a) for a in DEPS_ASPECTS]
diff --git a/node_modules/@angular/bazel/src/ng_package/ng_package.bzl b/node_modules/@angular/bazel/src/ng_package/ng_package.bzl
index 48d7619..ea635bb 100755
--- a/node_modules/@angular/bazel/src/ng_package/ng_package.bzl
+++ b/node_modules/@angular/bazel/src/ng_package/ng_package.bzl
@@ -13,9 +13,8 @@ It packages your library following the Angular Package Format, see the
specification of this format at https://goo.gl/jB3GVv
"""

-load("@build_bazel_rules_nodejs//internal/common:collect_es6_sources.bzl", "collect_es6_sources")
-load("@build_bazel_rules_nodejs//internal/common:node_module_info.bzl", "NodeModuleSources")
-load("@build_bazel_rules_nodejs//internal/common:sources_aspect.bzl", "sources_aspect")
+load("@build_bazel_rules_nodejs//:providers.bzl", "JSEcmaScriptModuleInfo", "JSNamedModuleInfo")
+load("@build_bazel_rules_nodejs//internal/common:npm_package_info.bzl", "NpmPackageInfo")
load(
"@build_bazel_rules_nodejs//internal/rollup:rollup_bundle.bzl",
"ROLLUP_ATTRS",
@@ -207,7 +206,12 @@ def _filter_js_inputs(all_inputs):
def _ng_package_impl(ctx):
npm_package_directory = ctx.actions.declare_directory("%s.ng_pkg" % ctx.label.name)

- esm_2015_files = _filter_out_generated_files(collect_es6_sources(ctx), "js")
+ esm_2015_files = []
+ for dep in ctx.attr.deps:
+ if JSEcmaScriptModuleInfo in dep:
+ esm_2015_files += dep[JSEcmaScriptModuleInfo].sources.to_list()
+
+ esm_2015_files = _filter_out_generated_files(esm_2015_files, "js")
esm5_sources = _filter_out_generated_files(flatten_esm5(ctx), "js")

# These accumulators match the directory names where the files live in the
@@ -347,9 +351,9 @@ def _ng_package_impl(ctx):
node_modules_files = _filter_js_inputs(ctx.files.node_modules)

# Also include files from npm fine grained deps as inputs.
- # These deps are identified by the NodeModuleSources provider.
+ # These deps are identified by the NpmPackageInfo provider.
for d in ctx.attr.deps:
- if NodeModuleSources in d:
+ if NpmPackageInfo in d:
node_modules_files += _filter_js_inputs(d.files)
esm5_rollup_inputs = depset(node_modules_files, transitive = [esm5_sources])

@@ -469,7 +473,7 @@ def _ng_package_impl(ctx):
files = depset([package_dir]),
)]

local_deps_aspects = [collect_node_modules_aspect, _collect_summaries_aspect]
-DEPS_ASPECTS = [esm5_outputs_aspect, sources_aspect]
+DEPS_ASPECTS = [esm5_outputs_aspect]

# Workaround skydoc bug which assumes ROLLUP_DEPS_ASPECTS is a str type
[DEPS_ASPECTS.append(a) for a in ROLLUP_DEPS_ASPECTS]
23 changes: 12 additions & 11 deletions internal/common/devmode_js_sources.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
Outputs a manifest file with the sources listed.
"""

load(":sources_aspect.bzl", "sources_aspect")
load("@build_bazel_rules_nodejs//:providers.bzl", "JSNamedModuleInfo")

# Avoid using non-normalized paths (workspace/../other_workspace/path)
def _to_manifest_path(ctx, file):
Expand All @@ -27,26 +27,27 @@ def _to_manifest_path(ctx, file):
return ctx.workspace_name + "/" + file.short_path

def _devmode_js_sources_impl(ctx):
files = depset()

for d in ctx.attr.deps:
if hasattr(d, "node_sources"):
files = depset(transitive = [files, d.node_sources])
elif hasattr(d, "files"):
files = depset(transitive = [files, d.files])
sources_depsets = []
for dep in ctx.attr.deps:
if JSNamedModuleInfo in dep:
sources_depsets.append(dep[JSNamedModuleInfo].sources)
if hasattr(dep, "files"):
sources_depsets.append(dep.files)
sources = depset(transitive = sources_depsets)

ctx.actions.write(ctx.outputs.manifest, "".join([
_to_manifest_path(ctx, f) + "\n"
for f in files.to_list()
for f in sources.to_list()
if f.path.endswith(".js") or f.path.endswith(".mjs")
]))
return [DefaultInfo(files = files)]

return [DefaultInfo(files = sources)]

devmode_js_sources = rule(
implementation = _devmode_js_sources_impl,
attrs = {
"deps": attr.label_list(
allow_files = True,
aspects = [sources_aspect],
),
},
outputs = {
Expand Down
74 changes: 0 additions & 74 deletions internal/common/node_module_info.bzl

This file was deleted.

52 changes: 52 additions & 0 deletions internal/common/npm_package_info.bzl
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
# Copyright 2017 The Bazel Authors. All rights reserved.
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

"""NpmPackageInfo providers and apsect to collect node_modules from deps.
Copy link
Collaborator

Choose a reason for hiding this comment

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

seems like it shouldn't be in internal/ - maybe providers.bzl or next to it?

Copy link
Collaborator Author

@gregmagolan gregmagolan Oct 8, 2019

Choose a reason for hiding this comment

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

providers.bzl seems like the right place as I don't think we want to introduce too many load points. where do you think for node_modules_aspect?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

kick this downstream to later PR

"""

# NpmPackageInfo provider is provided by targets that are npm dependencies by the
# `node_module_library` rule as well as other targets that have direct or transitive deps on
# `node_module_library` targets via the `node_modules_aspect` below.
NpmPackageInfo = provider(
doc = "Provides information about npm dependencies",
Copy link
Collaborator

Choose a reason for hiding this comment

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

will need a bunch more doc here as it should be a public API

fields = {
"direct_sources": "Depset of direct source files in this npm package",
"sources": "Depset of direct & transitive source files in this npm package and in its dependencies",
"workspace": "The workspace name that this npm package is provided from",
},
)

def _node_modules_aspect_impl(target, ctx):
providers = []

# provide NpmPackageInfo if it is not already provided there are NpmPackageInfo deps
if not NpmPackageInfo in target:
sources_depsets = []
workspace = None
if hasattr(ctx.rule.attr, "deps"):
for dep in ctx.rule.attr.deps:
if NpmPackageInfo in dep:
if workspace and dep[NpmPackageInfo].workspace != workspace:
fail("All npm dependencies need to come from a single workspace. Found '%s' and '%s'." % (workspace, dep[NpmPackageInfo].workspace))
workspace = dep[NpmPackageInfo].workspace
sources_depsets.append(dep[NpmPackageInfo].sources)
if workspace:
providers.extend([NpmPackageInfo(direct_sources = depset(), sources = depset(transitive = sources_depsets), workspace = workspace)])

return providers

node_modules_aspect = aspect(
_node_modules_aspect_impl,
attr_aspects = ["deps"],
)
Loading