build(aio): migrate examples to CLI #19248

Foxandxss · 2017-09-18T13:40:00Z

This small PR migrates all the examples infrastructure to CLI (prose will come in a different PR).

Since it is quite big, I am going to explain some commits and the reasoning behind them.

reestructure boilerplate to allow more types - Modified the boilerplate to allow different kind of boilerplates. AKA systemjs and CLI (we still have some SystemJS examples like the AOT chapter)

remove cli-quickstart boilerplate - CLI had boilerplate on it, removed it so it will always receive the latest boilerplate available.

put webpack as systemjs temporarily - This chapter is going to be possibly dropped (it doesn't appear on the sidebar anymore) and it was messing with my work, so I put it as systemjs as a way to make it not bother me.

remove webpack packages due conflict - CLI is a bit picky if you have webpack and other dependencies as well (all the packages for all examples are in the same bucket). I removed them and since we are going to remove the webpack chapter, it is just an early cleanup.

tune up all examples && modify each example to be CLI compliant - All examples needed changes. Cleanup of the HTML to remove systemjs artifacts, main.ts needs to load environments, and a few more extra changed that some chapters needed.

change examples to have app- prefix - We used to have my- as a prefix but the default for CLI is app and the idea is to have our examples as default as possible. You may notice that some concrete chapters doesn't have a prefix, but it makes sense to leave it as is for the chapter. Can be changed tho.

e2e support done - Yay, super fast. Some ignored examples, read below.

plunker done - It keeps working as it should, as systemjs examples.

zipper done - I realized that there are no more systemjs zips. So we could decide about dropping the support of systemjs and simplify the tool a bit.

That being said, there are some things that to notice here and to be addressed in following PRs:

No changes or minimum changes to ToH. They are going to be addressed in a different PR since Jules started a good PR for that.
There are some ignored e2e examples. quickstart (to be deleted), http (to be deleted), setup (to be deleted. Webpack(to be deleted),upgrade-phonecat` they are erroring quite a bit and I think it was an old issue that has nothing to do with this PR so we can fix it later.
Testing chapter has 1-2 non passing tests, but the original one had broken tests as well, so to be fixed in a separete PR by @wardbell.
i18n hasn't been touch. It needs some work to be translated to CLI both in code and prose. So they will be done separately.
Animations will fail e2e, there is a PR to fix that here at docs: animations - replace iterator with simple code style #18965.
Deployment as well as i18n hasn't been touch. Needs work. To be done as a separate PR.

petebacondarwin · 2017-09-18T14:49:59Z

This small PR ...

😝

petebacondarwin · 2017-09-18T15:06:13Z

aio/tools/examples/shared/yarn.lock

@@ -1,5 +1,7 @@
 # THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY.
 # yarn lockfile v1
+# yarn v0.25.3
+# node v7.8.0


probably didn't mean to include this change in this commit?

I am not touching this file but I guess that every time I use yarn add or yarn upgrade it will modify the comments.

Indeed. To ensure consistency, you must use the same version of yarn that we use on CI (v0.24.6 for now).

petebacondarwin · 2017-09-18T15:07:00Z

aio/tools/examples/shared/boilerplate/example-config.json

-{
-  "build": "build",
-  "run": "serve"
-}


I am guessing that this file was never copied as it didn't appear in the original list above.

Someone added that file there but it doesn't do anything, won't be copied with the tool. Like a "hey, this is how it could looks like, but it is only an example"

petebacondarwin

build(aio): reestructure boilerplate to allow more types

reestructure has one too many es.
allow more types of what?
It would seem more expandable for the example config to be projectType or something, which would be, at this stage, either "systemjs" or "cli", rather than the systemjs boolean flag.

petebacondarwin

build(aio): remove cli-quickstart boilerplate

LGTM - although adding the explanation to the commit message would be helpful.

Foxandxss · 2017-09-18T15:13:42Z

My reasoning behind the example-config not having the projectType is because we decided in the past to leave the example-config as empty as possible and just adding what is different in the project. That is why some of them have a build and run options (when it is not default) and the systemjs when it is not CLI.

petebacondarwin · 2017-09-18T15:19:34Z

You can still have the default as "projectType": "cli" so it would only need to appear for the SystemJS examples.

petebacondarwin

build(aio): add upgrade as a systemjs project

I think the commit message should be "... configure upgrade projects to use systemjs".
LGTM bearing in mind my suggestion for the projectType configuration property.

petebacondarwin

build(aio): put webpack as systemjs temporarily

message could be "... configure webpack example as systemjs" and then explain that it is a temporary hack as in the PR description.
LGTM, the irony of "run": "serve:cli", then "systemjs": true.

petebacondarwin

build(aio): add basic CLI artfacts

Is this in some way tied to a version of the CLI? What is the process for upgrading the CLI version?
Should we consider getting these boilerplate files directly from the CLI as needed, somehow, rather than checking them into the project?
Why are we copying the .gitignore' boilerplate (and then ignoring it as boilerplate)? Shouldn't we just include the ignore rules in our standard example .gitignorefile and not have this.gitignore` file in the boilerplate?

petebacondarwin

build(aio): remove webpack packages due conflict

LGTM

petebacondarwin

build(aio): update router to CLI to play with it

LGTM

petebacondarwin

build(aio): add assets and environments

LGTM

gkalpak

Nice! 👍 Moving all examples to cli will be awesome. (Even more so, if we automatically upgrade to the latest version 😁)

I left a few (random) comments from a quick look 😁

gkalpak · 2017-09-18T15:20:45Z

aio/content/examples/.gitignore

+**/e2e/tsconfig.e2e.json
+**/.angular-cli.json
+**/.editorconfig
+**/.gitignore


Very meta 😛

gkalpak · 2017-09-18T15:22:00Z

aio/content/examples/ajs-quick-reference/e2e/app.e2e-spec.ts

@@ -83,7 +83,7 @@ describe('AngularJS to Angular Quick Reference Tests', function () {
  }

  function testPosterButtonClick(expectedButtonText: string, isDisplayed: boolean) {
-    let posterButton = element(by.css('movie-list tr > th > button'));
+    let posterButton = element(by.css('app-movie-list tr > th > button'));


!@#$%^&* style guide 😛

gkalpak · 2017-09-18T15:24:45Z

aio/content/examples/animations/src/app/hero-list-auto.component.ts

@@ -13,7 +13,7 @@ import {
 import { Heroes } from './hero.service';

 @Component({
-  selector: 'hero-list-auto',
+  selector: 'app-list-auto',


Wouldn't app-hero-list-auto be more clear?

And similar for the rest of this example.

Fixing, that was the first example I did and did sound good but doesn't.

gkalpak · 2017-09-18T15:31:14Z

aio/content/examples/attribute-directives/src/app/app.component.1.html

@@ -1,14 +1,14 @@
 <!-- #docregion -->
 <h1>My First Attribute Directive</h1>
 <!-- #docregion applied -->
-<p myHighlight>Highlight me!</p>
+<p appHightlight>Highlight me!</p>


my is a way cooler prefix 😞

@gkalpak suggest it to the CLI team as the new default

gkalpak · 2017-09-18T16:05:27Z

aio/content/examples/ngcontainer/src/index.html

-      System.import('main.js').catch(function(err){ console.error(err); });
-    </script>
-  </head>
-
  <body>
    <my-app>Loading...</my-apps>


<my-app>?
Loading...?
😱

Oh. this chapter is in the list of the dead. Will be removed.

gkalpak · 2017-09-19T08:26:52Z

aio/tools/examples/shared/boilerplate/cli/src/polyfills.ts

@@ -46,7 +46,7 @@ import 'core-js/es7/reflect';
 * Required to support Web Animations `@angular/animation`.
 * Needed for: All but Chrome, Firefox and Opera. http://caniuse.com/#feat=web-animation
 **/
-// import 'web-animations-js';  // Run `npm install --save web-animations-js`.
+ import 'web-animations-js';  // Run `npm install --save web-animations-js`.


Shouldn't this be included in this package.json as a dependency?

Not reaally because it is not doing much. The interesting bits of that package.json are the scripts that comes with it. I copy them to each example for the sake of "This is a CLI package.json" but all dependencies are in the main package.json with all packages.

But yeah, I can add it.

That being said, I put that package for the zipper, which I forgot :)

gkalpak · 2017-09-19T08:27:42Z

aio/tools/examples/shared/boilerplate/cli/tsconfig.json

@@ -13,7 +12,7 @@
      "node_modules/@types"
    ],
    "lib": [
-      "es2016",
+      "es2017",


Why this change?

I am not sure why we have that, but the CLI tsconfig has es2017

gkalpak · 2017-09-19T08:37:51Z

aio/tools/plunker-builder/builder.js

@@ -148,6 +149,11 @@ class PlunkerBuilder {
        }
      }

+      // Matches main.ts or main.1.ts
+      if (/main(\.?|\-*?)\w*?\.ts/.test(relativeFileName)) {


I think the following RegExp you be more accurate:

/^main(?:[.-]\w+)?\.ts$/

+1 for the $/ at the end

Thanks, my regexp-fu is not that good

gkalpak · 2017-09-19T08:41:45Z

aio/tools/plunker-builder/translator/rules/mainTs.js

+    to: ''
+  },
+  environment_check: {
+    from: /if \(environment\.production\)\s*{\n*\s*enableProdMode\(\);\s*\n*}/mg,


\s matches \n, so no need for \n* around \s*.
Also, the m flag seems unnecessary (since you are not using ^ or $).

gkalpak · 2017-09-19T08:52:19Z

aio/tools/examples/run-example-e2e.js

  return globby(e2eSpecGlob, { cwd: basePath, nodir: true })
    .then(paths => paths
-      .filter(file => IGNORED_EXAMPLES.some(ignored => !file.startsWith(ignored)))
+      .filter(file => !IGNORED_EXAMPLES.some(ignored => file.startsWith(ignored)))


That didn't work at first so I had to change it after a while playing dumb with the javascript console heh

petebacondarwin

** build(aio): clean the cli-quickstart a bit more**:

LGTM

petebacondarwin · 2017-09-19T09:29:17Z

aio/tools/examples/shared/boilerplate/cli/src/polyfills.ts

@@ -46,7 +46,7 @@ import 'core-js/es7/reflect';
 * Required to support Web Animations `@angular/animation`.
 * Needed for: All but Chrome, Firefox and Opera. http://caniuse.com/#feat=web-animation
 **/
-// import 'web-animations-js';  // Run `npm install --save web-animations-js`.
+ import 'web-animations-js';  // Run `npm install --save web-animations-js`.


~~// Run npm install --save web-animations-js.~~

petebacondarwin

build(aio): more changes to cli boilerplate

LGTM except the removal of the unnecessary comment

filipesilva · 2017-09-19T09:31:27Z

aio/tools/examples/example-boilerplate.js

@@ -13,6 +13,9 @@ const TESTING_BASE_PATH = path.resolve(EXAMPLES_BASE_PATH, 'testing');

 const BOILERPLATE_PATHS = {
  cli: [
+    'src/environments/environment.prod.ts',


You can just remove the environment files if you want really. Remove them from the cli json as well and they won't be used.

No, I don't want to. They all end in the zipper later and I want them to be as exact as possible.

petebacondarwin · 2017-09-19T09:34:09Z

aio/content/examples/ngmodule/src/index.0.html

-    <script src="node_modules/systemjs/dist/system.src.js"></script>
-    <script src="systemjs.config.js"></script>
-    <script>
-      System.import('main.0.js').catch(function(err){ console.error(err); });


How do these index pages work now? I guess that the CLI is going to do nothing with them. Is it that they are never actually run?

Foxandxss · 2017-09-19T09:36:50Z

Each example is a CLI application that consist on index.html + main.ts. If any example has an extra index or main file, say index.0.html or index-final.html they are not part of the main application which also ends in the zip. They are just "snapshots" of the application to be shown in a plunker. That explains why they still have all the systemjs boilerplate on them. They are never meant to be run locally or with CLI

petebacondarwin

build(aio): migrate examples to CLI

LGTM although I wonder about those ngmodule examples

petebacondarwin · 2017-09-19T09:41:31Z

aio/content/examples/styleguide/src/04-12/app/core/nav/nav.component.css

  background-image: url("assets/ng.png");
  background-repeat: no-repeat;
  background-position: center center;
  padding: 1em 2em;
-}
+} */


why comment this out when you deleted it in aio/content/examples/styleguide/src/04-11/app/core/nav/nav.component.css?

petebacondarwin

build(aio): fix systemjs testing:

This commit needs some more explanation. Why these changes?

petebacondarwin

build(aio): testing is now cli:

I think this commit message should be more imperative. E.g. "Use CLI for unit testing examples", with more explanation in the message body.
LGTM otherwise

petebacondarwin · 2017-09-19T11:58:55Z

aio/tools/plunker-builder/translator/fileTranslator.js

@@ -5,6 +5,10 @@ function translate(html, rulesFile) {
    var rxRule = rulesFile.rules[rxDatum.pattern];
    // rxFrom is a rexexp
    var rxFrom = rxRule.from;
+    // check if there is an exception
+    if (rxDatum.exceptIf) {
+      if (html.indexOf(rxDatum.exceptIf) !== -1) return;


might be more flexible in the future for exceptIf to be a regex?

I guess you are right. But it seems I am not that good with regexp so I accept suggestions

petebacondarwin

build(aio): plunker done:

A couple of minor comments, including supporting @gkalpak's suggestions about regex changes.
LGTM otherwise

petebacondarwin · 2017-09-19T12:30:01Z

aio/tools/example-zipper/exampleZipper.js

+    const filePath = path.join(sourceFolder, EXAMPLE_CONFIG_NAME);
+    try {
+      const json = require(filePath, 'utf-8');
+      return json.systemjs ? 'systemjs' : 'cli';


this code would become simpler if we switched from the boolean flag to a projectType flag.

petebacondarwin

build(aio): zipper done:

LGTM (although it would benefit from the projectType flag discussed previously.

petebacondarwin · 2017-09-20T17:22:41Z

aio/content/examples/testing/src/app/app.module.ts

@@ -6,8 +6,7 @@ import { AppRoutingModule } from './app-routing.module';

 import { AboutComponent }   from './about.component';
 import { BannerComponent }  from './banner.component';
-import {  UserService }      from './model/user.service';
-import { Hero } from './model/hero';
+import { UserService }      from './model/user.service';
 import { HeroService } from './model/hero.service';


If you are going to arrange the columns then you may as well do this one too :-)

petebacondarwin · 2017-09-21T09:45:47Z

@Foxandxss - still a few comments unaddressed; and we now have a conflict 😱

mary-poppins · 2017-09-21T11:32:20Z

You can preview 48a496d at https://pr19248-48a496d.ngbuilds.io/.

mary-poppins · 2017-09-21T12:22:08Z

You can preview 52e8e31 at https://pr19248-52e8e31.ngbuilds.io/.

IgorMinar · 2017-09-21T23:57:26Z

aio/tools/examples/shared/boilerplate/cli/src/polyfills.ts

@@ -46,7 +46,7 @@ import 'core-js/es7/reflect';
 * Required to support Web Animations `@angular/animation`.
 * Needed for: All but Chrome, Firefox and Opera. http://caniuse.com/#feat=web-animation
 **/
-// import 'web-animations-js';  // Run `npm install --save web-animations-js`.
+ import 'web-animations-js';


why bring this in by default? I think this is wrong.

We have an animations chapter and a router chapter with animations.

The boilerplate is generic so if any chapter needs something, we add it for all of them.

At least is that how the tools works for now.

IgorMinar

I'm going to approve this and merge this into master. There is still some followup work to be done though - e.g. I left a comment about the web animations polyfill.

I'm not going to cherry-pick this into the stable branch yet because that would go live rightaway, which is likely not good since the prose is missing. Please let me know if I'm wrong about this.

Foxandxss · 2017-09-22T00:11:10Z

I don't think this PR will make the prose any different at all. On the one hand is better because now people will download actual CLI projects with a ZIP but on the other hand if a guide has a prefix hardcoded on it and now it is different, that could be a bit bad.

angular-automatic-lock-bot · 2019-09-12T17:21:12Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

Foxandxss added the comp: aio label Sep 18, 2017

Foxandxss requested review from petebacondarwin, wardbell, IgorMinar, filipesilva and juleskremer September 18, 2017 13:40

googlebot added the cla: yes label Sep 18, 2017

petebacondarwin reviewed Sep 18, 2017

View reviewed changes

gkalpak reviewed Sep 19, 2017

View reviewed changes

petebacondarwin added this to REVIEW in docs-infra Sep 19, 2017

petebacondarwin reviewed Sep 19, 2017

View reviewed changes

filipesilva reviewed Sep 19, 2017

View reviewed changes

petebacondarwin reviewed Sep 19, 2017

View reviewed changes

petebacondarwin reviewed Sep 20, 2017

View reviewed changes

petebacondarwin approved these changes Sep 20, 2017

View reviewed changes

petebacondarwin added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Sep 21, 2017

Foxandxss force-pushed the aio-cli-boilerplate branch from e663d67 to 48a496d Compare September 21, 2017 11:24

build(aio): change examples to CLI

52e8e31

Foxandxss force-pushed the aio-cli-boilerplate branch from 48a496d to 52e8e31 Compare September 21, 2017 12:12

petebacondarwin added action: merge The PR is ready for merge by the caretaker and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Sep 21, 2017

petebacondarwin moved this from REVIEW to MERGE in docs-infra Sep 21, 2017

petebacondarwin added the target: patch This PR is targeted for the next patch release label Sep 21, 2017

IgorMinar reviewed Sep 21, 2017

View reviewed changes

IgorMinar approved these changes Sep 22, 2017

View reviewed changes

IgorMinar added target: major This PR is targeted for the next major release and removed target: patch This PR is targeted for the next patch release labels Sep 22, 2017

IgorMinar closed this in 9bbf009 Sep 22, 2017

gkalpak removed this from MERGE in docs-infra Sep 25, 2017

trotyl mentioned this pull request Jan 8, 2018

Supporting a huge number of similar applications; Supporting software production lines (SPLs) #21352

Closed

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 12, 2019

build(aio): migrate examples to CLI #19248

build(aio): migrate examples to CLI #19248

Conversation

Foxandxss commented Sep 18, 2017

petebacondarwin commented Sep 18, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petebacondarwin left a comment

Choose a reason for hiding this comment

petebacondarwin left a comment

Choose a reason for hiding this comment

Foxandxss commented Sep 18, 2017

petebacondarwin commented Sep 18, 2017

petebacondarwin left a comment

Choose a reason for hiding this comment

petebacondarwin left a comment

Choose a reason for hiding this comment

petebacondarwin left a comment

Choose a reason for hiding this comment

petebacondarwin left a comment

Choose a reason for hiding this comment

petebacondarwin left a comment

Choose a reason for hiding this comment

petebacondarwin left a comment

Choose a reason for hiding this comment

gkalpak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petebacondarwin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petebacondarwin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Foxandxss commented Sep 19, 2017

petebacondarwin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petebacondarwin left a comment

Choose a reason for hiding this comment

petebacondarwin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petebacondarwin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petebacondarwin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petebacondarwin commented Sep 21, 2017

mary-poppins commented Sep 21, 2017

mary-poppins commented Sep 21, 2017

Choose a reason for hiding this comment

petebacondarwin commented Sep 18, 2017 •

edited