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(bazel): add an ng_package rule #22221

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
6 participants
@alexeagle
Contributor

alexeagle commented Feb 14, 2018

No description provided.

@mary-poppins

This comment has been minimized.

mary-poppins commented Feb 14, 2018

@mary-poppins

This comment has been minimized.

mary-poppins commented Feb 14, 2018

@mary-poppins

This comment has been minimized.

mary-poppins commented Feb 14, 2018

@mary-poppins

This comment has been minimized.

mary-poppins commented Feb 14, 2018

@alexeagle alexeagle requested a review from chuckjaz Feb 16, 2018

@mary-poppins

This comment has been minimized.

mary-poppins commented Feb 16, 2018

@mary-poppins

This comment has been minimized.

mary-poppins commented Feb 16, 2018

@chuckjaz

Some things to consider...

if f.endswith("/index.ts"):
if not index_file or len(f) < len(index_file):
index_file = f
tsconfig["files"] = [index_file]
if public_api_file:

This comment has been minimized.

@chuckjaz

chuckjaz Feb 16, 2018

Member

Consider making this supplied as a parameter instead of found by naming convention.

This comment has been minimized.

@alexeagle

alexeagle Feb 16, 2018

Contributor

sure, added entry_point as an optional attribute on ng_module

path.join(out, dirName, `${baseName}.d.ts`),
// Format carefully to match existing build.sh output
`/**
* @license Angular v${version}

This comment has been minimized.

@chuckjaz

chuckjaz Feb 16, 2018

Member

Shouldn't this be configurable? I assume not everyone that uses ng_package will want the same license file.

This comment has been minimized.

@alexeagle

alexeagle Feb 16, 2018

Contributor

yes, there's already a license header input to the action, I should just print that content here.

@mary-poppins

This comment has been minimized.

mary-poppins commented Feb 17, 2018

@mary-poppins

This comment has been minimized.

mary-poppins commented Feb 17, 2018

alexeagle added a commit to alexeagle/bazel-builds that referenced this pull request Feb 18, 2018

alexeagle added a commit to alexeagle/bazel-builds that referenced this pull request Feb 18, 2018

@mary-poppins

This comment has been minimized.

mary-poppins commented Feb 20, 2018

@mary-poppins

This comment has been minimized.

mary-poppins commented Feb 21, 2018

@mary-poppins

This comment has been minimized.

mary-poppins commented Feb 21, 2018

@mary-poppins

This comment has been minimized.

mary-poppins commented Feb 21, 2018

@mary-poppins

This comment has been minimized.

mary-poppins commented Feb 21, 2018

@mary-poppins

This comment has been minimized.

mary-poppins commented Feb 21, 2018

@mary-poppins

This comment has been minimized.

mary-poppins commented Feb 21, 2018

@mary-poppins

This comment has been minimized.

mary-poppins commented Feb 22, 2018

@mary-poppins

This comment has been minimized.

mary-poppins commented Feb 22, 2018

@mary-poppins

This comment has been minimized.

mary-poppins commented Feb 22, 2018

@mary-poppins

This comment has been minimized.

mary-poppins commented Feb 22, 2018

@@ -0,0 +1,270 @@
# Copyright Google Inc. All Rights Reserved.

This comment has been minimized.

@IgorMinar

IgorMinar Feb 22, 2018

Member

reviewed up to here. so far all looks good. I'll do the rest tomorrow.

This comment has been minimized.

@alexeagle

alexeagle Feb 22, 2018

Contributor

okay thanks I'll just sit here waiting :)

@alexeagle alexeagle referenced this pull request Feb 23, 2018

Open

Bazel testing #6

@IgorMinar

This looks good enough. let's merge it as is and follow up with more PRs to convert all the packages.

WELL_KNOWN_GLOBALS = {
"@angular/core": "ng.core",
"@angular/common": "ng.common",
"@angular/platform-browser": "ng.platformBrowser",

This comment has been minimized.

@IgorMinar

IgorMinar Feb 23, 2018

Member

this is not a complete list. this will not be sufficient to support material, and other packages.

args.add(["--output.format", format])
args.add(["--name", ctx.label.name])
# TODO(i): consider "--sourcemap", "inline"

This comment has been minimized.

@IgorMinar

IgorMinar Feb 23, 2018

Member

remove todo? (but keep the Note below)

bundles = []
for entry_point in [''] + ctx.attr.secondary_entry_points:
# TODO(alexeagle): index.js should be part of the secondary_entry_points

This comment has been minimized.

@IgorMinar

IgorMinar Feb 23, 2018

Member

is this still relevant? what's the action item? I don't understand.

] if p])
if entry_point:
# TODO jasonaden says there is no particular reason these filenames differ

This comment has been minimized.

@IgorMinar

IgorMinar Feb 23, 2018

Member

yeah. this is lame. we should change this to make things more consistent but let's land this as is first.

# for "@angular/core" and "http" for "@angular/common/http".
# The directory layout is:
# [x] <primary-entry-point>/

This comment has been minimized.

@IgorMinar

IgorMinar Feb 23, 2018

Member

these checkboxes seem outdated

# [x] <secondary-entry-point>.d.ts
# [x] <secondary-entry-point>.metadata.json
# [x] <secondary-entry-point>/
# [ ] package.json

This comment has been minimized.

@IgorMinar

IgorMinar Feb 23, 2018

Member

this is done

# [ ] package.json
# [x] <secondary-entry-point>.d.ts
# [x] <secondary-entry-point>.metadata.json
# [ ] bundles/

This comment has been minimized.

@IgorMinar
# [ ] <secondary-entry-point>.umd.js
# [ ] <secondary-entry-point>.umd.js.map
# [ ] <secondary-entry-point>.umd.min.js
# [ ] <secondary-entry-point>.umd.min.js.map

This comment has been minimized.

@IgorMinar

IgorMinar Feb 23, 2018

Member

all of the above is done

# [ ] <primary-entry-point>.js
# [ ] <primary-entry-point>.js.map
# [ ] <secondary-entry-point>.js
# [ ] <secondary-entry-point>.js.map

This comment has been minimized.

@IgorMinar

IgorMinar Feb 23, 2018

Member

all of the above is done

@mary-poppins

This comment has been minimized.

mary-poppins commented Feb 23, 2018

feat(bazel): add an ng_package rule
This produces a directory following the Angular Package layout spec.

Includes integration test coverage by making a minimal ng_package in integration/bazel.
Unit tests verify the content of the @angular/core and @angular/common packages.

This doesn't totally match our current output, but is good enough to unblock some
early adopters.

It re-uses logic from the rollup_bundle rule in rules_nodejs. It should also
eventually have the .pack and .publish secondary targets like npm_package rule.
@mary-poppins

This comment has been minimized.

mary-poppins commented Feb 23, 2018

@vicb vicb closed this in b43b164 Feb 23, 2018

smdunn pushed a commit to smdunn/angular that referenced this pull request Feb 28, 2018

feat(bazel): add an ng_package rule (angular#22221)
This produces a directory following the Angular Package layout spec.

Includes integration test coverage by making a minimal ng_package in integration/bazel.
Unit tests verify the content of the @angular/core and @angular/common packages.

This doesn't totally match our current output, but is good enough to unblock some
early adopters.

It re-uses logic from the rollup_bundle rule in rules_nodejs. It should also
eventually have the .pack and .publish secondary targets like npm_package rule.

PR Close angular#22221

leo6104 added a commit to leo6104/angular that referenced this pull request Mar 25, 2018

feat(bazel): add an ng_package rule (angular#22221)
This produces a directory following the Angular Package layout spec.

Includes integration test coverage by making a minimal ng_package in integration/bazel.
Unit tests verify the content of the @angular/core and @angular/common packages.

This doesn't totally match our current output, but is good enough to unblock some
early adopters.

It re-uses logic from the rollup_bundle rule in rules_nodejs. It should also
eventually have the .pack and .publish secondary targets like npm_package rule.

PR Close angular#22221
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment