fix(compiler-cli): Use typescript to resolve modules for metadata generation

[jeffora]

The current module resolution for the metadata collector simply attaches .ts to the import/export path, which does not work if the path is using Node / CommonJS behavior to resolve to an index.ts file. This patch uses typescript's module resolution logic, and will attempt to load the original typescript file if this resolution returns a .js or .d.ts file

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?

[x] 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 metadata collector does not generate correct metadata when a file uses an implied "index" in an import or export path. In the following example, the import refers to a path ../tools that will be resolved by typescript as ../tools/index.ts. The metadata collector, however, simply appends .ts directly to the path string before attempting to find it to generate metadata, thus looking for ../tools.ts, which doesn't exist. This results in the metadata having no information for named entry ɵb, despite having it listed in imports and origins.

// app.component.ts
import { BrowserModule } from '@angular/platform-browser';
import { NgModule } from '@angular/core';

import { ToolModule } from '../tools';
import { AppComponent } from './app.component';


@NgModule({
  declarations: [
    AppComponent
  ],
  imports: [
    BrowserModule,
    ToolModule,
  ],
  providers: [],
  bootstrap: [AppComponent]
})
export class AppModule { }

// ../tools/index.ts
import { NgModule } from '@angular/core';

@NgModule({
  declarations: [],
  exports: [],
  imports: [],
  providers: [],
})
export class ToolModule {

}

{
  "__symbolic": "module",
  "version": 4,
  "metadata": {
    "AppModule": {
      "__symbolic": "class",
      "decorators": [
        {
          "__symbolic": "call",
          ...
          "arguments": [
            {
              ...
              "imports": [
                {
                  "__symbolic": "reference",
                  "module": "@angular/platform-browser",
                  "name": "BrowserModule",
                  "line": 13,
                  "character": 4
                },
                {
                  "__symbolic": "reference",
                  "name": "ɵb"
                }
              ],
              ...
            }
          ]
        }
      ],
      "members": {}
    },
    "ɵa": {
      "__symbolic": "class",
      ...
    }
  },
  "origins": {
    "AppModule": "./src/app/app.module",
    "ɵa": "./src/app/app.component",
    "ɵb": "./src/tools"
  },
  ...
}

Issue Number: N/A

What is the new behavior?

This PR introduces a change to use the typescript module resolver to find files for metadata instead of just appending .ts to the input path name. This has the benefit that it will follow the same conventions that the runtime module resolution will use, and supports correctly finding files in all existing situations, as well as all situations where the path actually points to an index file (i.e. ../tools will find ../tools.ts or ../tools/index.ts depending on how the project is structured).

With this change, correct metadata is generated for the same project as defined above - as seen below, ɵb now has metadata properly generated:

{
  "__symbolic": "module",
  "version": 4,
  "metadata": {
    "AppModule": {
      "__symbolic": "class",
      "decorators": [
        {
          "__symbolic": "call",
          ...
          "arguments": [
            {
              ...
              "imports": [
                {
                  "__symbolic": "reference",
                  "module": "@angular/platform-browser",
                  "name": "BrowserModule",
                  "line": 13,
                  "character": 4
                },
                {
                  "__symbolic": "reference",
                  "name": "ɵb"
                }
              ],
              ...
            }
          ]
        }
      ],
      "members": {}
    },
    "ɵa": {
      "__symbolic": "class",
      ...
    },
    "ɵb": {
      "__symbolic": "class",
      "decorators": [
        {
          "__symbolic": "call",
          "expression": {
            "__symbolic": "reference",
            "module": "@angular/core",
            "name": "NgModule",
            "line": 2,
            "character": 1
          },
          "arguments": [
            {
              "declarations": [],
              "exports": [],
              "imports": [],
              "providers": []
            }
          ]
        }
      ],
      "members": {}
    }
  },
  "origins": {
    "AppModule": "./src/app/app.module",
    "ɵa": "./src/app/app.component",
    "ɵb": "./src/tools"
  },
  ...
}

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

I don't believe this closes any open issue with this repo currently, although it is possibly a start for #21082. However, I believe this is part of the root cause behind a range of issues in other libraries, including ng-packagr:

• TypeScript barrels break build ng-packagr/ng-packagr#195
• [AoT] Bundled metadata does not reflect metadata lowering // [PREV: Packaged library is not working in AOT mode] ng-packagr/ng-packagr#382

[jeffora]

Seems like a test failure in Travis was due to running out of space - I don't think that has anything to do with this PR, as nothing here writes to disk.

[ngbot]

Hi @jeffora! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

[jeffora]

It would be great if someone could get some eyes on this, even if just to say it's an unwanted change cc @matsko

[ngbot]

Hi @jeffora! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

[jeffora]

Updated this again, would be really great to get someone from the team to comment on this cc @matsko

[hakimio]

@alxhub Would be really nice if you could review this pull request. It's a rather serious issue.

[mhevery]

@jeffora I am sorry for the delay on getting back you. @alxhub is the best person to look at it and I have pinged him. In the meantime could you get the PR green, it is currently failing our CI.

[alxhub]

LGTM other than my one request. Thank you for doing this work!

[alxhub]

I think for safety, we should fall back on the old logic if resolveModuleName returns undefined. That is, we should append .ts to the file name in that case. That way we won't break existing apps if they somehow depend on our old broken behavior.

[jeffora]

Updated this to fallback to old behavior if the module is not resolved.

[alxhub]

http://test/OCL:203492592:BASE:203492621:1530897039126:ba045805

[jeffora]

@mhevery No problem! @alxhub I've updated to meet your changes, I'll keep an eye on the CI builds, but everything passed locally.

[ngbot]

I see that you just added the PR action: merge label, but the following checks are still failing:
     missing required label: "PR target: *"
     forbidden label detected: PR action: review
     status "ci/circleci: test" is failing
     status "code-review/pullapprove" is pending
     status "google3" is pending

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

[mhevery]

@jeffora It currently fails CI. Can you have a look.

[admosity]

Some weird error? Is there some imposed way to import here?

packages/compiler-cli/test/metadata/bundler_spec.ts:15:59 - error TS2307: Cannot find module '../mocks'.

15 import {Directory, MockAotContext, MockCompilerHost} from '../mocks';
                                                             ~~~~~~~~~~

packages/compiler-cli/test/metadata/bundler_spec.ts
packages/compiler-cli/test/mocks.ts

[jeffora]

@mhevery I've tried to recreate this locally, but I'm getting a bunch of different bazel errors. I've taken a guess at what I think the fix is and will see what happens with CI now. However, if it fails, some feedback or pointers for bazel would be great.

[alxhub]

http://test/OCL:203492592:BASE:203841881:1531175473311:9624a123

[jakeclarklucentmobile]

Sorry if this is a dumb question, need a little help.
How can I use this fix now that it has been approved?
I currently have Angular Cli 6 installed via npm.

Do I need to pull this repo and build it manually or can I update my npm package somehow to obtain these changes?

[alan-agius4]

You should wait for the next Angular release which will include this change, and update your package.json version to point to the lastest once’s.

[jakeclarklucentmobile]

Wonderful, do we have any idea when the next release will be cut? Is there a schedule?
Sorry again for silly questions. Thanks for your help

[jasonaden]

@jakeclarklucentmobile It should be cut some time tomorrow morning. We generally cut a release once a week on Wednesdays.

[dherges]

🆖 🚤

[ianchi]

@jeffora / @mhevery , I'm still having problems with barrel files.

I'm not 100% sure, but I think that this patch solves first level of barrel files, but the folder nesting is somehow lost, and when searching for nested imports in the resolved barrel, it treats the folder_with_barrel as a filename and it gets lost in the path for children:

Lets say:
parent_folder/folder_with_barrel

Gets correctly resolved, but the relative imports from inside folder_with_barrel/index.ts, are treated as relative to parent_folder. As if the folder_with_barrel part where a filename and not a folder.
And thus all those files are not found, and no metadata generated for them.

[aburht]

Is this issue resolved?

[aburht]

I'm still facing this issue with angular 7

[reinzwei]

@jeffora / @mhevery , I'm still having problems with barrel files.

I'm not 100% sure, but I think that this patch solves first level of barrel files, but the folder nesting is somehow lost, and when searching for nested imports in the resolved barrel, it treats the folder_with_barrel as a filename and it gets lost in the path for children:

Lets say:
parent_folder/folder_with_barrel

Gets correctly resolved, but the relative imports from inside folder_with_barrel/index.ts, are treated as relative to parent_folder. As if the folder_with_barrel part where a filename and not a folder.
And thus all those files are not found, and no metadata generated for them.

I am experiencing exactly same issue.

[angular-automatic-lock-bot]

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