Conversation
4afc8c1 to
7e72330
Compare
6cc948b to
43e60fa
Compare
alexeagle
left a comment
There was a problem hiding this comment.
I didn't review all the .ts code
There was a problem hiding this comment.
why do you need this change?
There was a problem hiding this comment.
why is this file in core/src/render3 ? shouldn't it be in a more common location?
There was a problem hiding this comment.
we can move it later, right now just keeping it in sync with Ivy repository
There was a problem hiding this comment.
if you add a trailing comma, the formatter will put one-per-line as you want
There was a problem hiding this comment.
This should not be needed, because glob should not descend into subpackages.
Apparently it's broken in bazel 0.8 but will be fixed in next release, according to bazelbuild/bazel#4242
There was a problem hiding this comment.
omit srcs, it's no longer required
There was a problem hiding this comment.
why do you need to change platform-server?
2ba4572 to
8c5c14d
Compare
|
I took care of most of these with exception of couple which would break the
original repo. I want to keep the code working in both places until we
officially cut over.
…On Thu, Dec 7, 2017 at 1:50 PM, Alex Eagle ***@***.***> wrote:
***@***.**** commented on this pull request.
I didn't review all the .ts code
------------------------------
In package.json
<#20855 (comment)>:
> @@ -91,7 +92,7 @@
"semver": "5.4.1",
"shelljs": "^0.7.8",
"source-map": "0.5.7",
- "source-map-support": "0.4.18",
+ "source-map-support": "^0.5.0",
why do you need this change?
------------------------------
In package.json
<#20855 (comment)>:
> @@ -41,6 +41,7 @@
***@***.***/node": "6.0.88",
***@***.***/selenium-webdriver": "3.0.7",
***@***.***/source-map": "^0.5.1",
+ ***@***.***/source-map-support": "^0.4.0",
do you need this?
------------------------------
In packages/core/src/render3/assert.ts
<#20855 (comment)>:
> @@ -0,0 +1,39 @@
+/**
+ * @license
+ * Copyright Google Inc. All Rights Reserved.
+ *
+ * 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
+ */
+
+function stringify(value: any) {
why is this file in core/src/render3 ? shouldn't it be in a more common
location?
------------------------------
In packages/core/src/render3/component.ts
<#20855 (comment)>:
> +
+
+/**
+ * Options which control how the component should be bootstrapped.
+ */
+export interface CreateComponentOptionArgs {
+ /**
+ * Which renderer to use.
+ */
+ renderer?: Renderer3;
+
+ rendererFactory?: RendererFactory3;
+
+ /**
+ * Which host element should the component be bootstrapped on. If not specified
+ * the component definition's `tag` is used t query the existing DOM for the
t -> to
------------------------------
In packages/core/src/render3/component.ts
<#20855 (comment)>:
> +
+ rendererFactory?: RendererFactory3;
+
+ /**
+ * Which host element should the component be bootstrapped on. If not specified
+ * the component definition's `tag` is used t query the existing DOM for the
+ * element to bootstrap.
+ */
+ host?: RElement|string;
+
+ /**
+ * Optional Injector which is the Module Injector for the component.
+ */
+ injector?: Injector;
+
+ features?: (<T>(component: T, componentDef: ComponentDef<T>) => void)[];
jsdoc?
------------------------------
In packages/core/src/render3/component.ts
<#20855 (comment)>:
> +
+/**
+ * Bootstrap a Component into an existing host element and return `ComponentRef`.
+ *
+ * @param componentType Component to bootstrap
+ * @param options Optional parameters which control bootstrapping
+ */
+export function createComponentRef<T>(
+ componentType: ComponentType<T>, opts: CreateComponentOptionArgs): ComponentRef<T> {
+ const component = renderComponent(componentType, opts);
+ const viewRef = createViewRef(detectChanges.bind(component), component);
+ return {
+ location: {nativeElement: getHostElement(component)},
+ injector: opts.injector || NULL_INJECTOR,
+ instance: component,
+ hostView: viewRef,
nit: name the variable the same as the object property so this is just
hostView (with no key)
------------------------------
In packages/core/src/render3/component.ts
<#20855 (comment)>:
> + return {
+ location: {nativeElement: getHostElement(component)},
+ injector: opts.injector || NULL_INJECTOR,
+ instance: component,
+ hostView: viewRef,
+ changeDetectorRef: viewRef,
+ componentType: componentType,
+ destroy: function() {},
+ onDestroy: function(cb: Function): void {}
+ };
+}
+
+function createViewRef<T>(detectChanges: () => void, context: T): EmbeddedViewRef<T> {
+ return addDestroyable(
+ {
+ rootNodes: null !,
should the type of rootNodes accept null?
------------------------------
In packages/core/src/render3/component.ts
<#20855 (comment)>:
> + return addDestroyable(
+ {
+ rootNodes: null !,
+ // inherited from core/ChangeDetectorRef
+ markForCheck: function(): void {
+ if (ngDevMode) {
+ implement();
+ }
+ },
+ detach: function(): void {
+ if (ngDevMode) {
+ implement();
+ }
+ },
+ detectChanges: detectChanges,
+ checkNoChanges: function(): void {
why don't these use fat arrow syntax?
------------------------------
In packages/core/src/render3/component.ts
<#20855 (comment)>:
> + destroy(): void;
+ onDestroy(cb: Function): void;
+}
+
+function implement() {
+ throw new Error('NotImplemented');
+}
+
+function addDestroyable<T, C>(obj: any, context: C): T&DestroyRef<C> {
+ let destroyFn: Function[]|null = null;
+ obj.destroyed = false;
+ obj.destroy = function() {
+ destroyFn && destroyFn.forEach((fn) => fn());
+ this.destroyed = true;
+ };
+ obj.onDestroy = function(fn: Function) { (destroyFn || (destroyFn = [])).push(fn); };
again, function syntax (we are missing linting in this repo)
------------------------------
In packages/core/src/render3/component.ts
<#20855 (comment)>:
> + throw new Error('NotImplemented');
+}
+
+function addDestroyable<T, C>(obj: any, context: C): T&DestroyRef<C> {
+ let destroyFn: Function[]|null = null;
+ obj.destroyed = false;
+ obj.destroy = function() {
+ destroyFn && destroyFn.forEach((fn) => fn());
+ this.destroyed = true;
+ };
+ obj.onDestroy = function(fn: Function) { (destroyFn || (destroyFn = [])).push(fn); };
+ return obj;
+}
+
+
+// TODO: A hack to not pull in the NullInjector from @angular/core.
can you fix the hack before landing in master?
------------------------------
In packages/core/src/render3/component.ts
<#20855 (comment)>:
> +
+/**
+ * Bootstrap a Component into an existing host element and return `NgComponent`.
+ *
+ * NgComponent is a light weight Custom Elements inspired API for bootstrapping and
+ * interacting with bootstrapped component.
+ *
+ * @param componentType Component to bootstrap
+ * @param options Optional parameters which control bootstrapping
+ */
+export function renderComponent<T>(
+ componentType: ComponentType<T>, opts: CreateComponentOptionArgs = {}): T {
+ const renderer = opts.renderer || document;
+ const componentDef = componentType.ngComponentDef;
+ let component: T;
+ const oldView = enterView(createViewState(-1, renderer), null !);
why doesn't the type of the last parameter admit null?
------------------------------
In packages/core/src/render3/di.ts
<#20855 (comment)>:
> + bloomBit < 64 ? (bloomBit < 32 ? di.cbf0 : di.cbf1) : (bloomBit < 96 ? di.cbf2 : di.cbf3);
+ // Only go to parent if parent may have value otherwise exit.
+ di = (value & mask) ? di.parent : null;
+ }
+ return null;
+}
+
+
+export function injectElementRef(): IElementRef {
+ let di = getOrCreateNodeInjector();
+ return di.elementRef || (di.elementRef = new ElementRef(di.node.native));
+}
+
+class ElementRef implements IElementRef {
+ readonly nativeElement: any;
+ constructor(nativeElement: any) { this.nativeElement = nativeElement; }
why any?
------------------------------
In packages/core/src/render3/index.ts
<#20855 (comment)>:
> @@ -0,0 +1,82 @@
+/**
+ * @license
+ * Copyright Google Inc. All Rights Reserved.
+ *
+ * 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
+ */
+
+// TODO: find out why this is needed to make closure compiler happy
why what is needed? the import statement?
------------------------------
In packages/core/src/render3/index.ts
<#20855 (comment)>:
> @@ -0,0 +1,82 @@
+/**
+ * @license
+ * Copyright Google Inc. All Rights Reserved.
+ *
+ * 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
+ */
+
+// TODO: find out why this is needed to make closure compiler happy
+import {createComponentRef, detectChanges, getHostElement, markDirty, renderComponent} from './component';
+import {inject, injectElementRef, injectTemplateRef, injectViewContainerRef} from './di';
+import {ComponentDef, ComponentTemplate, ComponentType, DirectiveDef, DirectiveDefFlags, NgOnChangesFeature, PublicFeature, defineComponent, defineDirective} from './public_interfaces';
+
+// Naming scheme:
+// - Capital letters are for crating things: T(Text), E(Element), D(Directive), V(View),
crating -> creating
------------------------------
In packages/core/src/render3/index.ts
<#20855 (comment)>:
> +
+ listenerCreate as L,
+ memory as m,
+ queryCreate as Q,
+
+ refreshComponent as r,
+ refreshContainer as rC,
+ refreshContainerEnd as rc,
+ refreshQuery as rQ,
+ textCreate as T,
+ textCreateBound as t,
+
+ viewCreate as V,
+ viewEnd as v
+} from './instructions';
+// clang-format on
if you add a trailing comma, the formatter will put one-per-line as you
want
------------------------------
In packages/core/test/BUILD.bazel
<#20855 (comment)>:
> @@ -6,7 +6,10 @@ ***@***.***_bazel_rules_nodejs//:defs.bzl", "jasmine_node_test")
ts_library(
name = "test_lib",
testonly = 1,
- srcs = glob(["**/*.ts"]),
+ srcs = glob(
+ ["**/*.ts"],
+ exclude = ["render3/**/*.ts"],
This should not be needed, because glob should not descend into
subpackages.
Apparently it's broken in bazel 0.8 but will be fixed in next release,
according to bazelbuild/bazel#4242
<bazelbuild/bazel#4242>
------------------------------
In packages/core/test/render3/BUILD.bazel
<#20855 (comment)>:
> + testonly = 1,
+ srcs = glob(
+ ["**/*.ts"],
+ exclude = ["**/*_perf.ts"],
+ ),
+ tsconfig = "//packages:tsconfig.json",
+ deps = [
+ "//packages:types",
+ "//packages/core",
+ "//packages/platform-browser",
+ ],
+)
+
+jasmine_node_test(
+ name = "render3",
+ srcs = [],
omit srcs, it's no longer required
------------------------------
In packages/platform-server/BUILD.bazel
<#20855 (comment)>:
> @@ -13,6 +13,7 @@ ts_library(
module_name = ***@***.***/platform-server",
tsconfig = "//packages:tsconfig",
deps = [
+ "//packages:types",
why do you need to change platform-server?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#20855 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAG1T3wtt6UdpzQ3v-0ff9suyavpMLK4ks5s-F3BgaJpZM4Q5BIs>
.
|
bd8d43b to
6e0ab34
Compare
| (actual < expected) && assertThrow(actual, expected, condition, '>'); | ||
| } | ||
|
|
||
| export function assertNotNull<T>(actual: T, condition: string) { |
| */ | ||
|
|
||
| function stringify(value: any) { | ||
| return typeof value === 'string' ? `"${value}"` : '' + value; |
There was a problem hiding this comment.
- Add an explanation (function description) for why string get wrapped in an additional set of
"but other types don't? stringifyisn't a great name because of that extra behavior of adding the"(vs. only coercing the type tostring). Could the name be more specific?
There was a problem hiding this comment.
Yeah, maybestringifyWithQuotes ?
There was a problem hiding this comment.
How about stringifyValueForDisplay or stringifyValueForError?
There was a problem hiding this comment.
Nice, let's do stringifyValueForError then
| * 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 | ||
| */ | ||
|
|
There was a problem hiding this comment.
Add a file-level comment that provides some context on the functions contained within? (what they're meant to be used for)
| (actual == expected) && assertThrow(actual, expected, condition, '!='); | ||
| } | ||
|
|
||
| export function assertThrow<T>( |
There was a problem hiding this comment.
Add JsDoc description block for this? It's not immediately obvious what condition, operator, and serializer are from just looking at this function.
(Ideally I'd like to see description on all the functions, but I'd acknowledge the rest are mostly self-explanatory)
| /** | ||
| * Options which control how the component should be bootstrapped. | ||
| */ | ||
| export interface CreateComponentOptionArgs { |
There was a problem hiding this comment.
Is this always for bootstrapping? If so, what about ComponentBootstrapConfig or ComponentBootstrapOptions?
If it's any creation, ComponentCreationConfig or ComponentCreationOptions?
It's not immediately clear what is meant by "OptionArgs".
| export interface CreateComponentOptionArgs { | ||
| /** | ||
| * Which renderer to use. | ||
| */ |
There was a problem hiding this comment.
Optional: short JsDoc can be collapsed to a single line
/** Renderer instance used for the created component. */
renderer?: Renderer3;This increases in information density of the file, making it easier to read more at once without scrolling.
| throw new Error('NotImplemented'); | ||
| } | ||
|
|
||
| function addDestroyable<T, C>(obj: any, context: C): T&DestroyRef<C> { |
There was a problem hiding this comment.
Add function description, especially for the params (obj: any could be anything).
There was a problem hiding this comment.
Could this be an arrow function?
There was a problem hiding this comment.
createComponentRef has the same description- change this function's description to be more specific about what it does?
There was a problem hiding this comment.
Yeah, it's a bit confusing because createComponentRef wraps renderComponent, so technically createComponentRef also does what's in here. The difference is really the return value. I'll update the comment.
There was a problem hiding this comment.
NgComponent is a lightweight API for bootstrapping and for interacting with the
bootstrapped component. The API is inspired by the native Custom Element API.
This functions returns T, not an NgComponent? Should there be a comment here to explain what's up with that?
There was a problem hiding this comment.
Ah, I believe that comment is out of date. It should say that it returns an instance of the component.
There was a problem hiding this comment.
Add comment that explains which error you're targeting with this try? Might also be good to comment on why try is okay here WRT potential VM optimization issues.
jelbourn
left a comment
There was a problem hiding this comment.
I ran out of steam for today. Will come back for more.
There was a problem hiding this comment.
Break this file up into more specific indivual files? Having one interface per file would probably be overkill, but breaking them up into smaller chunks would probably make this easier to navigate.
| const ngDevMode: boolean; | ||
| } | ||
|
|
||
| export const enum LNodeFlags { |
There was a problem hiding this comment.
Explain what these are and how they're used? Also, these seems to be a mix of flags, masks, and constants (the shifts), which is slightly odd.
| /** | ||
| * NOTES: | ||
| * | ||
| * Each Array costs 70 bytes and is composed of `Array` and `(array)` object |
There was a problem hiding this comment.
What is this note in reference to? It seems like it's describing a pattern somewhere in this file but it's not clear what exactly it's referring to.
There was a problem hiding this comment.
I think it's a general note to remind us of the cost when choosing between objects and arrays in the design phase. Where do you think a better place would be for it to go? It does seem out of place here, but don't know where to put it.
| /** | ||
| * `ViewState` stores all of the information needed to process the instructions as | ||
| * they are invoked from the template. `ViewState` is saved when a child `View` is | ||
| * being processed and restored when the child `View` is done. |
There was a problem hiding this comment.
I'm not what clear on what's meant by "saved" vs. "restored" here
| * need this to be able to efficiently find the `LView` when inserting the | ||
| * view into an anchor. | ||
| */ | ||
| readonly node: LView|LElement; |
There was a problem hiding this comment.
Comment says LView, but type says LView|LElement. Clarify?
Also, why not rootNode?
| values: any[]; | ||
| } | ||
|
|
||
| export class QueryState_ implements QueryState { |
There was a problem hiding this comment.
What's the reasoning behind having an interface when the concrete class is going to be QueryState_? Why not just have the implementation as QueryState and skip the interface? Without knowing the rationale it feels like an unnecessary level of indirection.
| */ | ||
|
|
||
| import {Observable} from 'rxjs/Observable'; | ||
| import {QueryList as IQueryList, Type} from '../core'; |
There was a problem hiding this comment.
Add comment here explaining the alias?
| @@ -0,0 +1,293 @@ | |||
| /** | |||
There was a problem hiding this comment.
More consistent function descriptions throughout this file?
| @@ -0,0 +1,156 @@ | |||
| /** | |||
There was a problem hiding this comment.
Function descriptions through this file?
| * found in the LICENSE file at https://angular.io/license | ||
| */ | ||
|
|
||
| import {ComponentFactory, ComponentRef as IComponentRef, ElementRef as IElementRef, EmbeddedViewRef as IEmbeddedViewRef, Injector, NgModuleRef as INgModuleRef, TemplateRef as ITemplateRef, Type, ViewContainerRef as IViewContainerRef, ViewRef as IViewRef} from '../core'; |
There was a problem hiding this comment.
Add comment explaining aliases here?
Would it make sense to pattern these aliases like LegacyComponentRef, LegacyElementRef, etc.? Just prefixing with an I doesn't make the code any more clear, it just mixes up the concept of prefixing all interfaces with I
80e9bd4 to
2b00efd
Compare
This PR fixes a circular dependency among those files in Renderer3: `query` -> `di` -> `instructions` -> `query` -> ... Looking at the above dependencies the `di` -> `instructions` import is a problematic one. Previously `di` had an import from `instructions` since we can known about "current node" only in `instructions` (and we need "current node" to create node injector instances). This commit refactors the code in the way that functions in the `di` file don't depend on any info stored module-global variables in `instructions`. PR Close #20855
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information