Ivy hello world with devserver #22788

mhevery · 2018-03-15T04:32:55Z

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] angular.io application / infrastructure changes
[ ] Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

[ ] Yes
[ ] No

Other information

googlebot · 2018-03-15T04:33:00Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

kara

LGTM aside from typo

kara · 2018-03-18T17:28:54Z

packages/core/test/bundling/hello_world/index.ts

 }
+//TODO(misko): Forgeting to export HellWorld and not having NgModule fails silently.


Typos: Forgeting -> Forgetting, HellWorld -> HelloWorld

vicb · 2018-03-18T22:43:30Z

WORKSPACE

-    url = "https://github.com/bazelbuild/rules_nodejs/archive/0.5.3.zip",
-    strip_prefix = "rules_nodejs-0.5.3",
-    sha256 = "17a5515f59777b00cb25dbc710017a14273f825029b2ec60e0969d28914870be",
+    url = "https://github.com/mhevery/rules_nodejs/archive/07c2cf1f04fa7b68220d730bc8df37ee9811b5e8.zip",


Why do you point to your repo here ?
Add comment with tracking issue.

That was a temporary work around until bazelbuild/rules_nodejs#158 got merged. It now points back to the main repo.

vicb · 2018-03-18T22:44:28Z

packages/core/src/render3/assert.ts

@@ -52,6 +52,13 @@ export function assertNotNull<T>(actual: T, msg: string) {
  }
 }

+export function assertComponentType(actual: any) {


other asserts have a msg parameter ? Would it make sense here ?

What would the message say other than Expecting ComponentType but got something else.

It could be msg || "Expecting ComponentType but got something else" and the callee could describe why there should be a component at that point. Would be consistent

vicb · 2018-03-18T22:45:04Z

packages/core/src/render3/component.ts

@@ -113,9 +114,13 @@ export const NULL_INJECTOR: Injector = {
 * @param options Optional parameters which control bootstrapping
 */
 export function renderComponent<T>(
-    componentType: ComponentType<T>, opts: CreateComponentOptions = {}): T {
+    componentType: ComponentType<T>|
+        Type<T>/* Type as workaround for: Microsoft/TypeScript/issues/4881 */


Should this be a TODO...

I don' think so. These are all over the code base, and there will have to be big clean up once that is resolved.

Should ComponentType be an union type with a TODO then ?

No, because then it would apply to all cases and we would lose type safety internally. We really just want it on the user code Ivy boundary.

Like a FromUserComponentType ?

vicb · 2018-03-18T22:46:40Z

packages/core/src/render3/component.ts

@@ -136,7 +141,8 @@ export function renderComponent<T>(
    // Create element node at index 0 in data array
    elementNode = hostElement(hostNode, componentDef);
    // Create directive instance with n() and store at index 1 in data array (el is 0)


look like this comment is a little out of sync with the code now ?

vicb · 2018-03-18T22:49:52Z

packages/core/test/bundling/hello_world/index.ts

  name = 'World';
+}
+// TODO(misko): Forgetting to export HelloWorld and not having NgModule fails silently.


tracking issue ?

I will be working on Ivy backlog and this will be included there. Don't have issue # yet.

vicb · 2018-03-18T22:51:35Z

tools/symbol-extractor/symbol_extractor.ts

-          fnDepth++;
-          if (fnDepth <= 1) {
-            // Only go into function expression once for the outer closure.
+          fnRecurseDepth++;


don't you ever need to -- ?

good catch, added.

do we need the test to be updated ?

For this to fail, we would have to have more than one top level closure, which does not happen in real life. Not sure that would be useful, what do you think.

I would update the test but it's up to you.

vicb · 2018-03-18T22:53:01Z

tools/symbol-extractor/symbol_extractor.ts

+ * Rollup produces this format when it wants to export symbols from a bundle.
+ * @param child
+ */
+function isStoringIIFE(child: ts.VariableDeclarationList): boolean {


isRollupExportSymbol() as it seems super specific to this use case ?

vicb · 2018-03-18T22:53:43Z

tools/symbol-extractor/symbol_extractor.ts

+  if (child.declarations.length !== 1) return false;
+  const decl: ts.VariableDeclaration = child.declarations[0];
+  if (decl.initializer && decl.initializer.kind == ts.SyntaxKind.CallExpression) {
+    return true;


nit: return !!decl.initializer && decl.initializer.kind == ts.SyntaxKind.CallExpression;

vicb · 2018-03-18T22:55:31Z

tools/symbol-extractor/symbol_extractor_spec/iife_with_export.js

+ * Use of this source code is governed by an MIT-style license that can be
+ * found in the LICENSE file at https://angular.io/license
+ */
+var fooBar = function(exports) {


Add a comment what this test is about (ie rollup export) ?

vicb

Changes mostly LGTM.
I'd like to see more comments.

vicb · 2018-03-19T21:11:48Z

tools/symbol-extractor/symbol_extractor.ts

-    return true;
-  }
-  return false;
+  return (decl.initializer && decl.initializer.kind == ts.SyntaxKind.CallExpression);


needs !! for boolean return type

argh! Fixed.

vicb

Thanks for the updates.

PR Close angular#22788

angular-automatic-lock-bot · 2019-09-13T10:52:11Z

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.}

mhevery requested review from chuckjaz and alxhub March 15, 2018 04:32

googlebot added the cla: no label Mar 15, 2018

mhevery force-pushed the ivy_hello_world branch from 95e23c0 to d01ca41 Compare March 15, 2018 04:37

trotyl mentioned this pull request Mar 15, 2018

[WIP] feat(compiler-cli): support RegExp literal in metadata #22283

Closed

3 tasks

mhevery force-pushed the ivy_hello_world branch 2 times, most recently from bb2cd4d to d0b3131 Compare March 18, 2018 15:52

mhevery changed the title ~~WIP: Ivy hello world~~ Ivy hello world with devserver Mar 18, 2018

mhevery added the target: major This PR is targeted for the next major release label Mar 18, 2018

mhevery force-pushed the ivy_hello_world branch from d0b3131 to f4f2494 Compare March 18, 2018 15:58

mhevery added cla: yes and removed cla: no labels Mar 18, 2018

kara approved these changes Mar 18, 2018

View reviewed changes

mhevery force-pushed the ivy_hello_world branch from 0d3a6ee to 0ac396d Compare March 18, 2018 19:01

vicb reviewed Mar 18, 2018

View reviewed changes

vicb suggested changes Mar 18, 2018

View reviewed changes

mhevery force-pushed the ivy_hello_world branch from 5e75738 to f0bcd9b Compare March 19, 2018 21:10

vicb reviewed Mar 19, 2018

View reviewed changes

mhevery added 3 commits March 19, 2018 14:46

test(ivy): switch HelloWorld to ivy compiler

5bcf297

fixup! test(ivy): switch HelloWorld to ivy compiler

cbcc6b8

fixup! test(ivy): switch HelloWorld to ivy compiler

a049f4d

fixup! test(ivy): switch HelloWorld to ivy compiler

5b1371f

mhevery force-pushed the ivy_hello_world branch from 135ffa9 to 5b1371f Compare March 19, 2018 21:46

vicb approved these changes Mar 19, 2018

View reviewed changes

fixup! test(ivy): switch HelloWorld to ivy compiler

9d1a8ed

mhevery added the action: merge The PR is ready for merge by the caretaker label Mar 19, 2018

mhevery closed this in fc50c77 Mar 19, 2018

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

test(ivy): switch HelloWorld to ivy compiler (angular#22788)

bdaecc6

PR Close angular#22788

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

		}
		//TODO(misko): Forgeting to export HellWorld and not having NgModule fails silently.

Ivy hello world with devserver #22788

Ivy hello world with devserver #22788

Conversation

mhevery commented Mar 15, 2018

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

googlebot commented Mar 15, 2018

kara 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vicb Mar 18, 2018 • 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

vicb left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vicb left a comment

Choose a reason for hiding this comment

angular-automatic-lock-bot bot commented Sep 13, 2019

vicb Mar 18, 2018 •

edited

vicb left a comment •

edited