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(core): introduce local scope & make NgModules optional in ivy #27481

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
8 participants
@mgechev
Member

mgechev commented Dec 5, 2018

PR Checklist

Please check if your PR fulfills the following requirements:

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?

The templates of a component are compiled based on the NgModule transitive scope, without accepting local component scope.

Issue Number: N/A

What is the new behavior?

The templates of the component could be only compiled using the local component scope, which makes NgModules optional.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@mgechev mgechev requested review from mhevery and alxhub Dec 5, 2018

@googlebot googlebot added the cla: yes label Dec 5, 2018

const expr = component.get('deps') !;
const declarationMeta = staticallyResolve(expr, this.reflector, this.checker);
localScope = resolveTypeList(expr, declarationMeta, 'deps', this.reflector)
.map(ref => new WrappedNodeExpr(ref));

This comment has been minimized.

@alxhub

alxhub Dec 5, 2018

Contributor

WrappedNodeExpr represents a TypeScript ts.Node as an Expression. This allows TS nodes to be passed directly through the @angular/compiler APIs (which know nothing about TS) and appear in generated expressions.

ref in this case is a Reference<ts.Declaration> which is the correct way to refer to a node in AOT. Reference supports .toExpression() to construct an Expression that will evaluate to the referenced type within the context of a particular source file (which might decide to use a WrappedNodeExpr of the type's ts.Identifier if appropriate).

I think localScope here should probably be of type Reference<ts.Declaration>[] instead of Expression[].

@@ -131,21 +131,22 @@ export class SelectorScopeRegistry {
this._pipeToName.set(node, name);
}
lookupCompilationScopeAsRefs(node: ts.Declaration): CompilationScope<Reference>|null {
lookupCompilationScopeAsRefs(node: ts.Declaration, localScope: Expression[]|null):

This comment has been minimized.

@alxhub

alxhub Dec 5, 2018

Contributor

I don't think this function should have any knowledge of localScope, but it does seem to be unavoidable with the current design.

I'll make a note to do some future refactoring here, but for now this should be fine (except the type should be Reference<ts.Declaration>[] as mentioned).

This comment has been minimized.

@mgechev

mgechev Dec 6, 2018

Member

Updated the implementation so that we don't have localScope.

let compilation: Reference<ts.Declaration>[] = [];
if (module) {
compilation =
this.lookupNgModuleScopesOrDie(module !, /* ngModuleImportedFrom */ null).compilation;

This comment has been minimized.

@alxhub

alxhub Dec 5, 2018

Contributor

There should be no need for the ! - TypeScript knows it can't be null/undefined here.

Also, prefer explicit conditionals: module !== null instead of module

@@ -13,6 +13,8 @@ import {TypeScriptReflectionHost} from '../../metadata';
import {getDeclaration, makeProgram} from '../../testing/in_memory_typescript';
import {ResourceLoader} from '../src/api';
import {ComponentDecoratorHandler} from '../src/component';
import {DirectiveDecoratorHandler} from '../src/directive';

This comment has been minimized.

@alxhub

alxhub Dec 5, 2018

Contributor

Make separate spec files for DirectiveDecoratorHandler and PipeDecoratorHandler.

This comment has been minimized.

@mgechev

mgechev Dec 5, 2018

Member

These imports were redundant, removed them.

/**
* A set of directives and pipes used in the component's template.
*/
deps?: Array<Type<any>>;

This comment has been minimized.

@alxhub

alxhub Dec 5, 2018

Contributor

What happens if this is used in applications compiled with View Engine? This shouldn't be allowed, and we should give an informative error message if someone tries.

@@ -71,8 +72,19 @@ export function compileComponent(type: Type<any>, metadata: Component): void {
// component may execute and set an ngSelectorScope property on the component type. This
// allows the component to patch itself with directiveDefs from the module after it
// finishes compiling.
if (hasSelectorScope(type)) {
if (hasSelectorScope(type) && metadata.deps) {

This comment has been minimized.

@alxhub

alxhub Dec 5, 2018

Contributor

Prefer explicit conditionals (metadata.deps !== undefined)

@@ -188,3 +200,18 @@ function isViewQuery(value: any): value is Query {
function splitByComma(value: string): string[] {
return value.split(',').map(piece => piece.trim());
}
function localScopeFor(deps: Array<Type<any>>): NgModuleTransitiveScopes {

This comment has been minimized.

@alxhub

alxhub Dec 5, 2018

Contributor

Since local scopes are just a set of directives and pipes to use in a single component and aren't available in a transitive context, there's really no reason this function should return NgModuleTransitiveScopes. Merely returning a set of directives and pipes is fine.

const scopes = transitiveScopesFor(type.ngSelectorScope);
const localScope = localScopeFor(metadata.deps);

This comment has been minimized.

@alxhub

alxhub Dec 5, 2018

Contributor

Can we restructure the logic here so we construct the overall scope first, and then call patchComponentDefWithScope unconditionally at the end?

This comment has been minimized.

@mgechev

mgechev Dec 5, 2018

Member

I updated the logic so we don't call patchComponentDefWithScope in each branch of the conditional statements. We still need to invoke it only when there are directives/pipes, otherwise directiveDefs and pipeDefs should be null.

This comment has been minimized.

@mgechev

mgechev Dec 6, 2018

Member

Removed the localScope and moved the deps.

@@ -23,5 +23,6 @@ jasmine_node_test(
deps = [
":ivy_lib",
"//packages/core/test/render3:domino",
"//packages/core/test/render3:render3_lib",

This comment has been minimized.

@alxhub

alxhub Dec 5, 2018

Contributor

We should break this target up so we're not pulling in the specs from //packages/core/test/render3:render3_lib as well. Maybe move it into //packages/core/test/render3:util?

/**
* Dependencies of the component's template which could be pipes or directives.
*/
localScope: o.Expression[]|null;

This comment has been minimized.

@alxhub

alxhub Dec 5, 2018

Contributor

I don't think this is needed anymore - we should be including the local dependencies into the directives and pipes sections of R3ComponentMetadata.

This comment has been minimized.

@mgechev

mgechev Dec 5, 2018

Member

I introduced localScope because during the analyze call we don't have metadata for the individual symbols and thus we'll break locality if we try to resolve it. Without the metadata we don't know if the symbols are directives or pipes.

@alxhub

This comment has been minimized.

Contributor

alxhub commented Dec 5, 2018

In addition to the specific review comments, I've been encouraging the team to take advantage of commit structure a lot more, and to record useful context in commit messages.

For this PR, I would like to see the commits split up into smaller functional pieces, that each implement a specific piece of the solution.

For example:

  • extracting resolveTypeList into a utility function could be its own refactor commit.
  • adding support for local scopes to selector_scope.ts could be its own feat commit (with accompanying local tests).
  • adding Component.deps along with the implementation JIT & AOT, and the accompanying higher level tests, could be its own feat commit. Ideally the implementation would just be wiring together logic added in previous impl commits.
  • finally, making the old compiler give a useful error could be its own commit.

Each of these commits should include not only the main commit title (first line), but a description of what is being implemented and why. I like to begin the description by talking about what state Angular was in beforehand, why it needed to change, and what specific changes the commit makes. For larger multi-commit PRs, I include a "Testing strategy" section in each commit that describes how the change will eventually be tested (usually when the code in the commit is tested as part of a larger operation in a later commit).

Recording this kind of information has a number of benefits:

  • It makes reviewing larger PRs a breeze, as the review can be done commit-by-commit (even by different reviewers if each commit touches a different part of the code).
  • When someone is reading the code down the road, they can easily use a tool in their IDE to read commit information for each line. Getting information out of the larger PR, on the other hand, is pretty difficult.
  • In the event of a regression, it's far more obvious what impact rolling the commit back will have.
  • It makes the release review for the caretaker much more straightforward, as they don't have to dig through the code to figure out what a specific change is about.

It's also a good idea (though not enforced by CI) that the tests should pass at each individual commit. This makes things like bisects for identifying performance regressions a lot more workable.

@mgechev

This comment has been minimized.

Member

mgechev commented Dec 5, 2018

The proposed PR structure makes sense to me and it should be part of the documentation (probably DEVELOPER.md?) We can discuss the right place for it in a different channel.

@mary-poppins

This comment has been minimized.

mary-poppins commented Dec 5, 2018

@angular angular deleted a comment from mary-poppins Dec 5, 2018

@angular angular deleted a comment from mary-poppins Dec 5, 2018

@JoostK

Awesome! 🎉

const def = (TestComponent as any).ngComponentDef;
expect(def).not.toBeNull();
const myComp = renderComponent(TestComponent as any);
const result = getHostElement(myComp).innerHTML;

This comment has been minimized.

@JoostK

JoostK Dec 5, 2018

Contributor

This logic is available as ComponentFixture (the Ivy one for tests, not the one for TestBed).

/**
* A set of directives and pipes used in the component's template.
*/
deps?: Array<Type<any>>;

This comment has been minimized.

@robwormald

robwormald Dec 5, 2018

Member

Should this take Array<Type<any>> | Array<Array<Type<any>>> and get smooshed, er, flatten'd?

Probably the broader question is that if we expect developers to be lazy, as developers are, presumably we'll see something like pre-RC5 usage:

//lib.ts
export class MyDirectiveA {}
export class MyDirectiveB {}

export const MY_DIRECTIVES = [
  MyDirectiveA, 
  MyDirectiveB
];
import {MY_DIRECTIVES} from './lib.ts'

@Directive()
export class AppDirective {}

@Component({
  deps: [AppDirective, ...MY_DIRECTIVES] //should users be responsible for spreading?
})
export class MyApp {}

This has implications on treeshaking of course.

This comment has been minimized.

@robwormald

robwormald Dec 5, 2018

Member

throwback to old idea: angular/angular-cli#96 (comment)

could potentially be optimized as a prod step?, which seems to be tree-shaking friendly: https://bit.ly/2PlrnSM

This comment has been minimized.

@robwormald

robwormald Dec 5, 2018

Member

All components are also directives, but that's something a new user has to learn out-of-band - mentioning components in the jsdoc explicitly here might be helpful?

This comment has been minimized.

@mgechev

mgechev Dec 6, 2018

Member

Added components as well. For the deps type, I'd go with Array<Type<any>>. IMO, it makes the API more obvious.

@mhevery may have an opinion on nested arrays.

This comment has been minimized.

@manekinekko

manekinekko Dec 6, 2018

Member

I do backup Rob's comment as I see that practice a lot in userland. Also, many developers will expect the deps property to work the same as the providers property.

This comment has been minimized.

@mgechev

mgechev Dec 6, 2018

Member

Looking at the pre-NgModules releases (rc.4), the corresponding properties were nested arrays as well. To keep consistent with providers and viewProviders it makes sense to update the type.

This comment has been minimized.

@mgechev

mgechev Dec 6, 2018

Member

It also makes sense to annotate deps with @internal until we decide how/if we want to ship this.

@mgechev mgechev force-pushed the mgechev:minko/optional-ng-modules branch 6 times, most recently from 9d78ce2 to 3fba7e7 Dec 6, 2018

@angular angular deleted a comment from mary-poppins Dec 6, 2018

@angular angular deleted a comment from mary-poppins Dec 6, 2018

@angular angular deleted a comment from mary-poppins Dec 6, 2018

@angular angular deleted a comment from mary-poppins Dec 6, 2018

@angular angular deleted a comment from mary-poppins Dec 6, 2018

@angular angular deleted a comment from mary-poppins Dec 6, 2018

@mary-poppins

This comment has been minimized.

mary-poppins commented Dec 6, 2018

@angular angular deleted a comment from mary-poppins Dec 6, 2018

@mgechev mgechev force-pushed the mgechev:minko/optional-ng-modules branch from 3fba7e7 to 2192263 Dec 6, 2018

@mary-poppins

This comment has been minimized.

mary-poppins commented Dec 6, 2018

@mgechev mgechev force-pushed the mgechev:minko/optional-ng-modules branch 2 times, most recently from 56d852e to f10045d Dec 6, 2018

@mgechev mgechev force-pushed the mgechev:minko/optional-ng-modules branch from f10045d to 348cc92 Dec 6, 2018

refactor: move resolveTypeList to utils
Move the `resolveTypeList` function to `utils` so it can be reused in
`Component` and `NgModule` decorator handlers.

@mgechev mgechev force-pushed the mgechev:minko/optional-ng-modules branch from 348cc92 to f7e7985 Dec 6, 2018

@angular angular deleted a comment from mary-poppins Dec 6, 2018

@angular angular deleted a comment from mary-poppins Dec 6, 2018

@angular angular deleted a comment from mary-poppins Dec 6, 2018

@angular angular deleted a comment from mary-poppins Dec 6, 2018

@mgechev mgechev force-pushed the mgechev:minko/optional-ng-modules branch from f7e7985 to 7ee210d Dec 6, 2018

@mary-poppins

This comment has been minimized.

mary-poppins commented Dec 6, 2018

function localScopeFor(deps: Array<Type<any>>) {
const pipes = new Set(deps.filter(dep => !!getPipeDef(dep)));
const directives = new Set(deps.filter(dep => !!getDirectiveDef(dep) || !!getComponentDef(dep)));
return {directives, pipes};

This comment has been minimized.

@trotyl

trotyl Dec 6, 2018

Contributor

It seems that JIT implementation supports multi-purpose class while AOT doesn't?

@Directive({})
@Pipe({})
class MyMultiPurposeClass implements PipeTransform { }

This comment has been minimized.

@mgechev

mgechev Dec 6, 2018

Member

Shouldn't be. Updated the implementation.

feat(core): add deps property to component
Introduce `deps` property in the configuration object passed to the `Component` decorator. The `deps` property contains a list of providers and directives which are used in the component's template. This makes components independent from NgModules and simplifies the API for `@angular/elements` and `@angular/core`.

@mgechev mgechev force-pushed the mgechev:minko/optional-ng-modules branch from 7ee210d to a1966ac Dec 6, 2018

@mary-poppins

This comment has been minimized.

mary-poppins commented Dec 6, 2018

@mgechev

This comment has been minimized.

Member

mgechev commented Dec 6, 2018

Closing the PR for now; we can reopen it once we land more critical blocking work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment