GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — fix(compiler-cli): Use typescript to resolve modules for metadata generation by jeffora · Pull Request #22856 · angular/angular

Conversation

jeffora commented Mar 19, 2018

Copy link

Copy Markdown

Contributor

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:

dherges/ng-packagr#195
dherges/ng-packagr#382

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

cnic3139, esyo, Traksewt, ssuperczynski, reven114, alfaproject, ericmorentin, vtabary, szymonbultrowicz, shayded-exe, and 13 more reacted with thumbs up emoji

albyrock87, tiagodws, and MatissJanis reacted with heart emoji

All reactions

👍 23 reactions
❤️ 3 reactions

googlebot added the cla: yes label

Mar 19, 2018

jeffora mentioned this pull request

Mar 19, 2018

TypeScript barrels break build ng-packagr/ng-packagr#195

Closed

jeffora force-pushed the fix-metadata-module-resolution branch from 93639be to f9ad283 Compare

March 19, 2018 03:27

jeffora commented Mar 19, 2018

Copy link

Copy Markdown

Contributor Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

matsko added the area: core Issues related to the framework runtime label

Mar 22, 2018

ngbot Bot commented Mar 27, 2018

Copy link

Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

jeffora force-pushed the fix-metadata-module-resolution branch from f9ad283 to 15b5de9 Compare

March 28, 2018 00:32

jeffora commented Mar 28, 2018

Copy link

Copy Markdown

Contributor Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ngbot Bot commented Apr 4, 2018

Copy link

Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

jeffora force-pushed the fix-metadata-module-resolution branch from 15b5de9 to 38e2266 Compare

April 11, 2018 05:22

jeffora commented Apr 11, 2018

Copy link

Copy Markdown

Contributor Author

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

elclanrs, kepennar, MatissJanis, admosity, and hakimio reacted with thumbs up emoji

All reactions

👍 5 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

IgorMinar requested a review from alxhub

May 15, 2018 23:29

IgorMinar added the action: review The PR is still awaiting reviews from at least one requested reviewer label

May 15, 2018

admosity mentioned this pull request

Jun 29, 2018

Apps cannot build with AOT if a dependent library was built with barrels #23713

Closed

hakimio commented Jul 5, 2018

Copy link

Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

hakimio mentioned this pull request

Jul 6, 2018

Production build (with "aot" enabled) fails when library is imported as a package allenhwkim/ngui-common#5

Closed

mhevery self-assigned this

Jul 6, 2018

mhevery commented Jul 6, 2018

Copy link

Copy Markdown

Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

alxhub requested changes

Jul 6, 2018

View reviewed changes

alxhub left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

packages/compiler-cli/src/metadata/bundler.ts Outdated

alxhub Jul 6, 2018

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

jeffora Jul 7, 2018

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

alxhub commented Jul 6, 2018

Copy link

Copy Markdown

Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

jeffora force-pushed the fix-metadata-module-resolution branch from 38e2266 to 512b7b8 Compare

July 7, 2018 00:02

jeffora commented Jul 7, 2018

Copy link

Copy Markdown

Contributor Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

mhevery added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels

Jul 7, 2018

ngbot Bot commented Jul 7, 2018

Copy link

Copy Markdown

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

mhevery added the target: patch This PR is targeted for the next patch release label

Jul 7, 2018

mhevery commented Jul 7, 2018

Copy link

Copy Markdown

Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

admosity commented Jul 9, 2018

Copy link

Copy Markdown

Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


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

1632c53

The current module resolution 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

jeffora force-pushed the fix-metadata-module-resolution branch from 512b7b8 to 1632c53 Compare

July 9, 2018 14:52

jeffora commented Jul 9, 2018

Copy link

Copy Markdown

Contributor Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

alxhub approved these changes

Jul 9, 2018

View reviewed changes

alxhub commented Jul 9, 2018

Copy link

Copy Markdown

Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

alexeagle approved these changes

Jul 10, 2018

View reviewed changes

mhevery pushed a commit that referenced this pull request

Jul 10, 2018


          fix(compiler-cli): Use typescript to resolve modules for metadata (#2…

7717ff1

…2856)

The current module resolution 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 Close #22856

mhevery closed this in

0d5f2d3

Jul 10, 2018

jakeclarklucentmobile commented Jul 10, 2018

Copy link

Copy Markdown

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?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

alan-agius4 commented Jul 10, 2018

Copy link

Copy Markdown

Contributor

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

jakeclarklucentmobile commented Jul 10, 2018

Copy link

Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

jasonaden commented Jul 10, 2018

Copy link

Copy Markdown

Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

victornoel mentioned this pull request

Jul 23, 2018

Function calls are not supported in decorators when fullTemplateTypeCheck is not specified and @dynamic has no effect #23609

Closed

danielsogl added a commit to danielsogl/angular that referenced this pull request

Jul 25, 2018


          docs: refactor pipe example to use the HttpClient

db21c38

docs: fix typos in 'Httpclient' docs (angular#19127)

PR Close angular#19127

docs(router): add `paramsInheritanceStrategy` documentation (angular#22590)

PR Close angular#22590

docs: add app.module to changed documents (angular#23876)

PR Close angular#23876

docs: clarify faqs about services (angular#24079)

PR Close angular#24079

docs(aio): added a link to Angular Zero online course (Traditional Chinese) (angular#24228)

PR Close angular#24228

docs: describe rounding behaviour of 'DecimalPipe' (angular#24303)

PR Close angular#24303

docs(common): fix in the documentation of PUT (angular#24528)
PR Close angular#24528

docs: add workspace and related cli terms (angular#24633)

PR Close angular#24633

docs(changelog): correct inaccuracies (angular#24713)
PR Close angular#24713

test: integration test for TS 2.9.x (angular#24749)

PR Close angular#24749

fix(ivy): pipes are pure by default (angular#24750)

PR Close angular#24750

build(bazel): update to rule_nodejs 0.10.0 (angular#24759)

PR Close angular#24759

build: upgrade jasmine (and related typings) to latest version (angular#19904)

With these changes, the types are a little stricter now and also not
compatible with Protractor's jasmine-like syntax. So, we have to also
use `@types/jasminewd2` for e2e tests (but not for non-e2e tests).

I also had to "augment" `@types/jasminewd2`, because the latest
typings from [DefinitelyTyped][1] do not reflect the fact that the
`jasminewd2` version (v2.1.0) currently used by Protractor supports
passing a `done` callback to a spec.

[1]: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/566e0394859fdc1dc893658ccec6b06372d56a91/types/jasminewd2/index.d.ts#L9-L15

Fixes angular#23952
Closes angular#24733

PR Close angular#19904

test: make `NgMatchers` type-aware (angular#19904)

PR Close angular#19904

refactor: infer type for `it()` assertion functions (angular#19904)

PR Close angular#19904

build: upgrade jasmine to 2.99.x and fix tests (angular#19904)

PR Close angular#19904

refactor: re-organize and "modernize" cjs-jasmine scripts (angular#19904)

PR Close angular#19904

test: run unit tests in random order (angular#19904)

PR Close angular#19904

build: upgrade jasmine to 3.1.0 (angular#19904)

PR Close angular#19904

build: upgrade karma and related dependencies (angular#19904)

PR Close angular#19904

docs: refactored ng-container code (angular#22742)


PR Close angular#22742

docs: update Angular Boot Camp description (angular#23653)

PR Close angular#23653

feat(service-worker): add support for `?` in SW config globbing (angular#24105)

The globbing is used in the following sections:
- `assetGroups` > `resources` > `files`/`versionedFiles`
- `assetGroups` > `resources` > `urls`
- `dataGroups` > `urls`
- `navigationUrls`

Query params are ignored for `files`/`versionedFiles` and
`navigationUrls`, but they are still taken into account for
`assetGroups`/`dataGroups` `urls`. To avoid a breaking change, `?` is
matched literally for these patterns.

PR Close angular#24105

refactor(service-worker): avoid unnecessary operations and remove unused code (angular#24127)

PR Close angular#24127

fix(service-worker): avoid network requests when looking up hashed resources in cache (angular#24127)

PR Close angular#24127

feat(ivy): properly apply style="", [style], [style.foo] and [attr.style] bindings (angular#24602)

PR Close angular#24602

fix(language-service): do not overwrite native `Reflect` (angular#24299)

Fixes angular#21420

PR Close angular#24299

fix(common): use correct currency format for locale de-AT (angular#24658)

Fixes angular#24609
PR Close angular#24658

fix(ivy): correctly resolve Array  property access (angular#24664)

PR Close angular#24664

build: make `internal-angular` karma reporter compatible with latest karma (angular#24803)

Due to changes in [email protected], `internal-angular` karma reporter stopped
showing browser logs (such as `console.log()` etc.).
Related to d571a51.

PR Close angular#24803

build: remove unnecessary `internal-angular` karma reporter (angular#24803)

The reporter was added in 87d56ac, with the purpose of fixing
source-map paths (which was apparently needed back then). Things have
moved around a lot since then and the custom reporter doesn't seem to be
necessary any more. By removing the reporter, we have one less thing to
worry about while upgrading karma; plus we get improvements in built-in
reporters for free.

Output with the custom reporter:
```
at someMethod (packages/core/.../some-file.ts:13:37)
```

Output with the built-in reporter:
```
at someMethod (packages/core/.../some-file.ts:13.37 <- dist/all/@angular/core/.../some-file.js:1:337)
```

PR Close angular#24803

fix(core): mark NgModule as not the root if APP_ROOT is set to false (angular#24814)

Tree shakable providers use the APP_ROOT token to determine where to attach themselves. APP_ROOT gets set on NgModule with BrowserModule irrespective of whether it is actually the root(Ex. in case of SSR app where the shell app is first bootstrapped without BrowserModule being the root module).

This change allows a NgModule with BrowserModule to explicitly mark itself as not the root by setting APP_ROOT token to false. This allows tree shakable providers to be attached to the right rott module.

PR Close angular#24814

fix(platform-browser): mark Meta and Title services as tree shakable providers (angular#24815)

This lets services that use Meta and Title services to be tree shakable and provided in root.

PR Close angular#24815

docs(aio): unified console.log single string style (angular#22737)
PR Close angular#22737

docs: unified console.log single string style (angular#22737)


PR Close angular#22737

fix(compiler-cli): Use typescript to resolve modules for metadata (angular#22856)

The current module resolution 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 Close angular#22856

docs: clarify wording in architecture overview (angular#24481)

Closes angular#23463
Closes angular#22158

PR Close angular#24481

docs: add tree-shakable providers (angular#24481)

PR Close angular#24481

fix(ivy): support projecting into dynamic views (angular#24752)

PR Close angular#24752

refactor(ivy): replace pNextOrParent with TNode props (angular#24752)

PR Close angular#24752

docs: fix typo in Universal guide (angular#24812)

PR Close angular#24812

fix(platform-browser): workaround wrong import path generated by ngc for DOCUMENT (angular#24830)


docs: correct project definition (angular#24807)

PR Close angular#24807

build: update to latest nodejs bazel rules (angular#24817)

PR Close angular#24817

fix(common): do not round factional seconds (angular#24831)

fixes angular#24384

PR Close angular#24831

docs: fix typos referencing inline component styles (angular#22557)

PR Close angular#22557

fix(router): add ability to recover from malformed url (angular#23283)

Fixes angular#21468

PR Close angular#23283

docs: fix incorrect forms selector references (angular#22631)

PR Close angular#22631

docs(aio): unified string chaining (angular#22735)
PR Close angular#22735

docs: unified string chaining (angular#22735)


PR Close angular#22735

docs(forms): added missing backtick (angular#24451)

Fixed trivial markdown problem with a missing backtick.

PR Close angular#24451

docs(forms): update API reference for form validators (angular#24734)

PR Close angular#24734

Merge remote-tracking branch 'upstream/master' into refactor-example-pipe


docs: refactor pipe example to use the HttpClient


docs: refactor pipe example to use the HttpClient


docs: refactor pipe example to use the HttpClient

IgorMinar pushed a commit that referenced this pull request

Jul 30, 2018


          docs: refactor pipe example to use the HttpClient (#22741)

5399c17

docs: fix typos in 'Httpclient' docs (#19127)

PR Close #19127

docs(router): add `paramsInheritanceStrategy` documentation (#22590)

PR Close #22590

docs: add app.module to changed documents (#23876)

PR Close #23876

docs: clarify faqs about services (#24079)

PR Close #24079

docs(aio): added a link to Angular Zero online course (Traditional Chinese) (#24228)

PR Close #24228

docs: describe rounding behaviour of 'DecimalPipe' (#24303)

PR Close #24303

docs(common): fix in the documentation of PUT (#24528)
PR Close #24528

docs: add workspace and related cli terms (#24633)

PR Close #24633

docs(changelog): correct inaccuracies (#24713)
PR Close #24713

test: integration test for TS 2.9.x (#24749)

PR Close #24749

fix(ivy): pipes are pure by default (#24750)

PR Close #24750

build(bazel): update to rule_nodejs 0.10.0 (#24759)

PR Close #24759

build: upgrade jasmine (and related typings) to latest version (#19904)

With these changes, the types are a little stricter now and also not
compatible with Protractor's jasmine-like syntax. So, we have to also
use `@types/jasminewd2` for e2e tests (but not for non-e2e tests).

I also had to "augment" `@types/jasminewd2`, because the latest
typings from [DefinitelyTyped][1] do not reflect the fact that the
`jasminewd2` version (v2.1.0) currently used by Protractor supports
passing a `done` callback to a spec.

[1]: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/566e0394859fdc1dc893658ccec6b06372d56a91/types/jasminewd2/index.d.ts#L9-L15

Fixes #23952
Closes #24733

PR Close #19904

test: make `NgMatchers` type-aware (#19904)

PR Close #19904

refactor: infer type for `it()` assertion functions (#19904)

PR Close #19904

build: upgrade jasmine to 2.99.x and fix tests (#19904)

PR Close #19904

refactor: re-organize and "modernize" cjs-jasmine scripts (#19904)

PR Close #19904

test: run unit tests in random order (#19904)

PR Close #19904

build: upgrade jasmine to 3.1.0 (#19904)

PR Close #19904

build: upgrade karma and related dependencies (#19904)

PR Close #19904

docs: refactored ng-container code (#22742)


PR Close #22742

docs: update Angular Boot Camp description (#23653)

PR Close #23653

feat(service-worker): add support for `?` in SW config globbing (#24105)

The globbing is used in the following sections:
- `assetGroups` > `resources` > `files`/`versionedFiles`
- `assetGroups` > `resources` > `urls`
- `dataGroups` > `urls`
- `navigationUrls`

Query params are ignored for `files`/`versionedFiles` and
`navigationUrls`, but they are still taken into account for
`assetGroups`/`dataGroups` `urls`. To avoid a breaking change, `?` is
matched literally for these patterns.

PR Close #24105

refactor(service-worker): avoid unnecessary operations and remove unused code (#24127)

PR Close #24127

fix(service-worker): avoid network requests when looking up hashed resources in cache (#24127)

PR Close #24127

feat(ivy): properly apply style="", [style], [style.foo] and [attr.style] bindings (#24602)

PR Close #24602

fix(language-service): do not overwrite native `Reflect` (#24299)

Fixes #21420

PR Close #24299

fix(common): use correct currency format for locale de-AT (#24658)

Fixes #24609
PR Close #24658

fix(ivy): correctly resolve Array  property access (#24664)

PR Close #24664

build: make `internal-angular` karma reporter compatible with latest karma (#24803)

Due to changes in [email protected], `internal-angular` karma reporter stopped
showing browser logs (such as `console.log()` etc.).
Related to d571a51.

PR Close #24803

build: remove unnecessary `internal-angular` karma reporter (#24803)

The reporter was added in 87d56ac, with the purpose of fixing
source-map paths (which was apparently needed back then). Things have
moved around a lot since then and the custom reporter doesn't seem to be
necessary any more. By removing the reporter, we have one less thing to
worry about while upgrading karma; plus we get improvements in built-in
reporters for free.

Output with the custom reporter:
```
at someMethod (packages/core/.../some-file.ts:13:37)
```

Output with the built-in reporter:
```
at someMethod (packages/core/.../some-file.ts:13.37 <- dist/all/@angular/core/.../some-file.js:1:337)
```

PR Close #24803

fix(core): mark NgModule as not the root if APP_ROOT is set to false (#24814)

Tree shakable providers use the APP_ROOT token to determine where to attach themselves. APP_ROOT gets set on NgModule with BrowserModule irrespective of whether it is actually the root(Ex. in case of SSR app where the shell app is first bootstrapped without BrowserModule being the root module).

This change allows a NgModule with BrowserModule to explicitly mark itself as not the root by setting APP_ROOT token to false. This allows tree shakable providers to be attached to the right rott module.

PR Close #24814

fix(platform-browser): mark Meta and Title services as tree shakable providers (#24815)

This lets services that use Meta and Title services to be tree shakable and provided in root.

PR Close #24815

docs(aio): unified console.log single string style (#22737)
PR Close #22737

docs: unified console.log single string style (#22737)


PR Close #22737

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

The current module resolution 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 Close #22856

docs: clarify wording in architecture overview (#24481)

Closes #23463
Closes #22158

PR Close #24481

docs: add tree-shakable providers (#24481)

PR Close #24481

fix(ivy): support projecting into dynamic views (#24752)

PR Close #24752

refactor(ivy): replace pNextOrParent with TNode props (#24752)

PR Close #24752

docs: fix typo in Universal guide (#24812)

PR Close #24812

fix(platform-browser): workaround wrong import path generated by ngc for DOCUMENT (#24830)


docs: correct project definition (#24807)

PR Close #24807

build: update to latest nodejs bazel rules (#24817)

PR Close #24817

fix(common): do not round factional seconds (#24831)

fixes #24384

PR Close #24831

docs: fix typos referencing inline component styles (#22557)

PR Close #22557

fix(router): add ability to recover from malformed url (#23283)

Fixes #21468

PR Close #23283

docs: fix incorrect forms selector references (#22631)

PR Close #22631

docs(aio): unified string chaining (#22735)
PR Close #22735

docs: unified string chaining (#22735)


PR Close #22735

docs(forms): added missing backtick (#24451)

Fixed trivial markdown problem with a missing backtick.

PR Close #24451

docs(forms): update API reference for form validators (#24734)

PR Close #24734

Merge remote-tracking branch 'upstream/master' into refactor-example-pipe


docs: refactor pipe example to use the HttpClient


docs: refactor pipe example to use the HttpClient


docs: refactor pipe example to use the HttpClient


PR Close #22741

dherges commented Sep 21, 2018

Copy link

Copy Markdown

Contributor

🆖 🚤

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ianchi commented Sep 27, 2018

Copy link

Copy Markdown

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

angular-buch-de and reinzwei reacted with thumbs up emoji

All reactions

👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

aburht commented Feb 8, 2019

Copy link

Copy Markdown

Is this issue resolved?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

aburht commented Feb 8, 2019

Copy link

Copy Markdown

I'm still facing this issue with angular 7

angular-buch-de, reinzwei, and spenceryue reacted with thumbs up emoji

All reactions

👍 3 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

reinzwei commented Apr 9, 2019

Copy link

Copy Markdown

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

Rlcolli4 and spenceryue reacted with thumbs up emoji

All reactions

👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

angular-automatic-lock-bot Bot commented Sep 14, 2019

Copy link

Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

angular-automatic-lock-bot Bot locked and limited conversation to collaborators

Sep 14, 2019

Reviewers

alxhub

alxhub approved these changes

+1 more reviewer

alexeagle

alexeagle approved these changes

Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

mhevery

Labels

action: merge

The PR is ready for merge by the caretaker

area: core

Issues related to the framework runtime

cla: yes target: patch

This PR is targeted for the next patch release

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

16 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.