feat(compiler-cli): add resource inlining to ngc #22615

alexeagle · 2018-03-06T22:53:43Z

When angularCompilerOptions { enableResourceInlining: true }, we replace all templateUrl / styleUrls properties in @Component with template / styles

mary-poppins · 2018-03-06T23:01:35Z

You can preview dd1a51d at https://pr22615-dd1a51d.ngbuilds.io/.

mary-poppins · 2018-03-06T23:05:47Z

You can preview 4b18d71 at https://pr22615-4b18d71.ngbuilds.io/.

alexeagle · 2018-03-07T00:02:05Z

@chuckjaz : the ngc_spec test fails because the tsickle transformer runs first and erases the decorators. I could register my transformer in main.ts instead, where I have access to the beforeTsickle argument to emitWithTsickle, does that seem right? Then when I do that, tsickle fails to transform the resulting sourcemap, maybe I have to do something special to fix them up?

: TypeError: Cannot read property 'text' of undefined
 at ClassDeclaration in /tmp/tmp.968164/my.component.ts:3:9
 at SourceFile in /tmp/tmp.968164/my.component.ts:2:9
    at Object.getTokenPosOfNode (/usr/local/google/home/alexeagle/Projects/angular/node_modules/typescript/lib/typescript.js:7515:72)
    at NodeObject.getStart (/usr/local/google/home/alexeagle/Projects/angular/node_modules/typescript/lib/typescript.js:94843:23)
    at /usr/local/google/home/alexeagle/Projects/src/transformer_sourcemap.ts:162:17
    at visitNodes (/usr/local/google/home/alexeagle/Projects/angular/node_modules/typescript/lib/typescript.js:12699:30)
    at Object.forEachChild (/usr/local/google/home/alexeagle/Projects/angular/node_modules/typescript/lib/typescript.js:12945:24)
    at NodeObject.forEachChild (/usr/local/google/home/alexeagle/Projects/angular/node_modules/typescript/lib/typescript.js:94981:23)
    at NodeSourceMapper.addMapping (/usr/local/google/home/alexeagle/Projects/src/transformer_sourcemap.ts:161:18)
    at DecoratorRewriter.Rewriter.writeRange (/usr/local/google/home/alexeagle/Projects/src/rewriter.ts:159:23)
    at DecoratorRewriter.Rewriter.writeLeadingTrivia (/usr/local/google/home/alexeagle/Projects/src/rewriter.ts:133:10)
    at DecoratorRewriter.maybeProcess (/usr/local/google/home/alexeagle/Projects/src/decorator-annotator.ts:377:14)
    at DecoratorRewriter.Rewriter.visit (/usr/local/google/home/alexeagle/Projects/src/rewriter.ts:72:17)
    at /usr/local/google/home/alexeagle/Projects/src/rewriter.ts:120:12
    at visitNodes (/usr/local/google/home/alexeagle/Projects/angular/node_modules/typescript/lib/typescript.js:12699:30)

alfaproject · 2018-03-07T21:01:27Z

@alexeagle will this respect TS path mapping?
ref: angular/angular-cli#9766

alexeagle · 2018-03-08T23:34:42Z

@alfaproject this uses the existing resource loader in ngc so the behavior should be the same - so no :(

mary-poppins · 2018-03-08T23:43:34Z

You can preview 191689a at https://pr22615-191689a.ngbuilds.io/.

When angularCompilerOptions { enableResourceInlining: true }, we replace all templateUrl and styleUrls properties in @component with template/styles

mary-poppins · 2018-03-08T23:53:45Z

You can preview 627be1d at https://pr22615-627be1d.ngbuilds.io/.

chuckjaz

NITs, otherwise LGTM

chuckjaz · 2018-03-09T00:01:49Z

packages/compiler-cli/src/transformers/inline_resources.ts

+
+  updateDecoratorMetadata(arg: MetadataObject): MetadataObject {
+    if (arg['templateUrl']) {
+      const templateUrl = arg['templateUrl'];


NIT: Consider factoring out the code duplication between templateUrl and styleUrls

chuckjaz · 2018-03-09T00:06:56Z

packages/compiler-cli/src/transformers/inline_resources.ts

+
+  const declaration = symbol.declarations[0] !;
+
+  if (!ts.isImportSpecifier(declaration)) {


Consider adding !declarations || here instead of the ! on 217.

chuckjaz · 2018-03-09T00:09:17Z

packages/compiler-cli/src/transformers/inline_resources.ts

+
+  const name = (declaration.propertyName || declaration.name).text;
+  const moduleId =
+      (declaration.parent !.parent !.parent !.moduleSpecifier as ts.StringLiteral).text;


Consider adding a comment that parents are guaranteed to be set in the AST given to a transformer and the depth of the tree is guaranteed by the the traversal by which arrives at this code.

chuckjaz · 2018-03-09T00:10:01Z

packages/compiler-cli/src/transformers/inline_resources.ts

+ */
+function updateComponentProperties(
+    args: ts.NodeArray<ts.Expression>, host: ResourceLoader): ts.NodeArray<ts.Expression> {
+  if (args.length !== 1) {


+1 for redundant checks as you can emit on error.

I don't follow - are you saying this code is good as written since it's tolerant of incorrect input?

chuckjaz · 2018-03-09T00:11:10Z

packages/compiler-cli/src/transformers/inline_resources.ts

+            return node;
+
+          default:
+            break;


return node;?

oops, missing a test for that

alfaproject · 2018-03-09T08:00:50Z

@alexeagle sorry if it's off-topic, but did you guys ever considered that? o:

alexeagle · 2018-03-09T14:19:25Z

@alfaproject I can understand how you might load resources dynamically, and have a different design-time module layout on disk than you have at runtime when fetching resources, and so you have exactly the same use case that TypeScript path mapping was designed for. And it reduces overall complexity for users if the various ways to express a dependency from one file on another use the same semantics. I think it's reasonable to file a feature request for that.

mary-poppins · 2018-03-09T14:35:17Z

You can preview d7a9dd6 at https://pr22615-d7a9dd6.ngbuilds.io/.

mary-poppins · 2018-03-09T14:47:13Z

You can preview 0148f5b at https://pr22615-0148f5b.ngbuilds.io/.

alexeagle · 2018-03-09T17:13:43Z

FYI
@juristr let's try this in your example lib as soon as it's live in the snapshot builds
@dherges you might want to use this in ng-packagr in place of your workaround, once you rely on dep on angular v6

juristr · 2018-03-09T17:41:16Z

@alexeagle oh, nice. Yep lets try it, currently I inlined the templates manually in the bazel branch

dherges

Hey Alex, it will be a big win!

dherges · 2018-03-10T06:53:16Z

packages/compiler-cli/src/transformers/inline_resources.ts

+import {MetadataTransformer, ValueTransform} from './metadata_cache';
+
+export type ResourceLoader = {
+  loadResource(path: string): Promise<string>| string;


👍 Perfect! The Promise<string> | string allows to integrate external tools like scss, less and other preprocessors preprocessors.

nope, if it doesn't return string this transform fails because the typescript compiler is a synchronous API. Under this toolchain the preprocessors are expected to run before ngc.

Hm 😐 Nevertheless, I'll get rid of the horrible replace source text by string-position-marker thing that you'll solved in a more elegant way in ngc.

dherges · 2018-03-10T07:03:51Z

packages/compiler-cli/src/transformers/inline_resources.ts

+  // The number of parent references here match the recursion depth at this point.
+  const moduleId =
+      (declaration.parent !.parent !.parent !.moduleSpecifier as ts.StringLiteral).text;
+  return moduleId === '@angular/core' && name === 'Component';


devkit has an AST util for resolving namespace imports. ng-packagr resolves namespaces too

thanks, Angular has a build-time dependency on devkit but not yet a runtime dependency... will have to think about that.
What's the failing test case look like?

Should be:

import * as ngCore from '@angular/core'; @ngCore.Component({ templateUrl: `..` }) export class Something {}

https://github.com/dherges/ng-packagr/blob/master/src/lib/ts/ng-ts-ast.spec.ts#L64-L66

dherges · 2018-03-10T07:04:59Z

packages/compiler-cli/src/transformers/inline_resources.ts

+    return false;
+  }
+
+  const name = (declaration.propertyName || declaration.name).text;


👍 I think this accounts for named imports – import { Component } from '@angular/core' – and aliased imports – import { Component as Foo } from '@angular/core'

I should add tests for those cases though - I borrowed some of this code from you but dropped all the conditional logic I wasn't testing

dherges · 2018-03-10T07:22:36Z

For users: the transformer's calls to loadResource() are forwarded to the compiler host's readResource()

alippai · 2018-03-12T09:55:47Z

Wasn't inlining already possible with the *ngfactory files? What's the difference with this PR?

alexeagle · 2018-03-12T16:16:25Z

@alippai other tools were doing inlining on the resulting JS outputs of the Angular compiler before. Now we make this a first-party feature. (Under Bazel, we never plan to do the workaround of modifying JS outputs) Also it means source-maps should be more correct.

alippai · 2018-03-12T16:39:56Z

@alexeagle awesome, thanks!

angular-automatic-lock-bot · 2019-09-13T10:21:50Z

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

alexeagle requested a review from chuckjaz March 6, 2018 22:53

googlebot added the cla: yes label Mar 6, 2018

alexeagle force-pushed the ngc_resource_inlining branch from dd1a51d to 4b18d71 Compare March 6, 2018 22:57

alexeagle added the area: core Issues related to the framework runtime label Mar 6, 2018

alexeagle force-pushed the ngc_resource_inlining branch from 4b18d71 to 191689a Compare March 8, 2018 23:33

feat(compiler-cli): add resource inlining to ngc

627be1d

When angularCompilerOptions { enableResourceInlining: true }, we replace all templateUrl and styleUrls properties in @component with template/styles

alexeagle force-pushed the ngc_resource_inlining branch from 191689a to 627be1d Compare March 8, 2018 23:44

chuckjaz approved these changes Mar 9, 2018

View reviewed changes

alexeagle added 2 commits March 9, 2018 06:23

fixup! feat(compiler-cli): add resource inlining to ngc

9c37ad1

fixup! feat(compiler-cli): add resource inlining to ngc

d7a9dd6

alexeagle added target: major This PR is targeted for the next major release action: merge The PR is ready for merge by the caretaker labels Mar 9, 2018

fixup! feat(compiler-cli): add resource inlining to ngc

0148f5b

alexeagle added action: merge The PR is ready for merge by the caretaker and removed action: merge The PR is ready for merge by the caretaker labels Mar 9, 2018

kara closed this in b5be18f Mar 9, 2018

dherges reviewed Mar 10, 2018

View reviewed changes

alfaproject mentioned this pull request Mar 10, 2018

Allow TypeScript path mapping in templateUrl angular/angular-cli#9766

Closed

Toxicable mentioned this pull request Mar 17, 2018

Add ngc option to inline templates for libraries. #16157

Closed

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


		const declaration = symbol.declarations[0] !;

		if (!ts.isImportSpecifier(declaration)) {

feat(compiler-cli): add resource inlining to ngc #22615

feat(compiler-cli): add resource inlining to ngc #22615

Uh oh!

Conversation

alexeagle commented Mar 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mary-poppins commented Mar 6, 2018

Uh oh!

mary-poppins commented Mar 6, 2018

Uh oh!

alexeagle commented Mar 7, 2018

Uh oh!

alfaproject commented Mar 7, 2018

Uh oh!

alexeagle commented Mar 8, 2018

Uh oh!

mary-poppins commented Mar 8, 2018

Uh oh!

mary-poppins commented Mar 8, 2018

Uh oh!

chuckjaz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alfaproject commented Mar 9, 2018

Uh oh!

alexeagle commented Mar 9, 2018

Uh oh!

mary-poppins commented Mar 9, 2018

Uh oh!

mary-poppins commented Mar 9, 2018

Uh oh!

alexeagle commented Mar 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

juristr commented Mar 9, 2018

Uh oh!

dherges left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dherges commented Mar 10, 2018

Uh oh!

alexeagle commented Mar 6, 2018 •

edited

Loading

alexeagle commented Mar 9, 2018 •

edited

Loading