GV

GeezerVison

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

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — feat(router): add ability to recover from malformed url by skryvets · Pull Request #23283 · angular/angular

Conversation

@skryvets

@skryvets skryvets commented Apr 10, 2018 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown

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?

Using invalid character sequences (like %) in the query param throws an exception that can't be catched anywhere and causing the app to crash.

Issue Number: 21468

What is the new behavior?

Code that might throw an exception is wrapped inside the try/catch construction preventing app from crash.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

Sorry, something went wrong.

Uh oh!

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

All reactions
@googlebot googlebot added the cla: yes label Apr 10, 2018
skryvets
skryvets commented Apr 10, 2018
View reviewed changes
Comment thread packages/router/src/router.ts Outdated

@skryvets skryvets Apr 10, 2018 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
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.

I changed it to use internal method parseUrl() which has try/catch construction when parsing the URL. Is that qualified as a breaking change?

Sorry, something went wrong.

Uh oh!

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

All reactions

@jasonaden jasonaden Apr 10, 2018

Copy link
Copy Markdown
Contributor

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.

No.

Sorry, something went wrong.

Uh oh!

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

All reactions
skryvets
skryvets commented Apr 10, 2018
View reviewed changes
Comment thread packages/router/src/router.ts Outdated

@skryvets skryvets Apr 10, 2018 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
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.

The defaultMalformedUriErrorHandler is redirecting to the root url in a case of an error. In order to comply with type MalformedUriErrorHandler it should have two params urlSerializer and url.
This is an example of the method call:

this.defaultMalformedUriErrorHandler(this.urlSerializer, '/');

However, I would like to avoid passing root url from outside. It seems more expected when the default behavior is defined inside the method:

function defaultMalformedUriErrorHandler(urlSerializer: UrlSerializer, url: string): UrlTree {
  return urlSerializer.parse('/');
}

If I would do that, I will have "unnecessary" url param (since it's not used by defaultHandler), but it's needed to stay in order to comply with the method type.

Question. What is better:

  1. Pass root url from outside.
  2. Implement it as an internal part by not using the url param.

Sorry, something went wrong.

Uh oh!

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

All reactions

@wKoza wKoza Apr 10, 2018

Copy link
Copy Markdown
Contributor

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.

Why do you want to add a param url in your type MalformedUriErrorHandler ? Is not it the role of the implementation to define the path ?

Sorry, something went wrong.

Uh oh!

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

All reactions

@skryvets skryvets Apr 10, 2018

Copy link
Copy Markdown
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.

So the user can configure it by providing their own handling function. Please see this part.

Sorry, something went wrong.

Uh oh!

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

All reactions

@wKoza wKoza Apr 10, 2018

Copy link
Copy Markdown
Contributor

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.

Ok, but in this case, we could simply have a your url in ExtraOptions like this:

interface ExtraOptions { 
  pathWhenMalformedUriError?: string // Sorry for the name 

and change your function by:

function malformedUriErrorHandler(urlSerializer: UrlSerializer, url: string): UrlTree {
  return urlSerializer.parse(url);
}

And your catch by:

urlTree = this.malformedUriErrorHandler(this.urlSerializer, this.pathWhenMalformedUriError);

Of course, Router has the default value ''.

Sorry, something went wrong.

Uh oh!

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

All reactions

@skryvets skryvets Apr 10, 2018 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
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.

@jasonaden, Could you please share you thoughts on this? Thank you!

Sorry, something went wrong.

Uh oh!

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

All reactions
@skryvets

skryvets commented Apr 10, 2018 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Author

I implemented this fix, but have few doubts (commented in the diff).

All reactions

Sorry, something went wrong.

Uh oh!

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

@skryvets
skryvets force-pushed the fix-malformed-url-error branch from a942298 to 8c13eea Compare April 10, 2018 10:55
jasonaden
jasonaden approved these changes Apr 10, 2018
View reviewed changes

@jasonaden jasonaden left a comment

Copy link
Copy Markdown
Contributor

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.

Overall this looks good. I made a couple minor adjustments (you'll see three fixup! commits which will get squashed when this is merged). I also ran the tools to update the golden public API file since this adds to the public APIs.

Overall this is great and will be quite helpful to the community. Thanks!

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread packages/router/src/router.ts Outdated

@jasonaden jasonaden Apr 10, 2018

Copy link
Copy Markdown
Contributor

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.

No.

Sorry, something went wrong.

Uh oh!

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

All reactions
@jasonaden jasonaden added the action: merge The PR is ready for merge by the caretaker label Apr 10, 2018
@ngbot

ngbot Bot commented Apr 10, 2018

Copy link
Copy Markdown

I see that you just added the PR action: merge label, but the following checks are still failing:
    failure missing required label: "PR target: *"
    failure missing required status "code-review/pullapprove"
    pending status "ci/circleci: build" is pending
    pending status "ci/circleci: lint" is pending
    pending status "google3" is pending
    pending status "continuous-integration/travis-ci/pr" 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.

@jasonaden jasonaden added type: bug/fix area: router labels Apr 10, 2018
@jasonaden jasonaden added this to the v6.0 milestone Apr 10, 2018
IgorMinar
IgorMinar previously requested changes Apr 11, 2018
View reviewed changes
Comment thread packages/router/src/router.ts Outdated

@IgorMinar IgorMinar Apr 11, 2018

Copy link
Copy Markdown
Contributor

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.

this is an old jsdoc tag, we no longer use it. please use @description instead. The first sentence should be descriptive and meaningful by itself. See other @description tags in this file for examples.

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread tools/public_api_guard/router/router.d.ts Outdated

@IgorMinar IgorMinar Apr 11, 2018

Copy link
Copy Markdown
Contributor

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.

maybe I don't understand what's going on here, but I find it weird that the custom url handler is exposed on the router class. Why is that needed? shouldn't the router call this handler when it needs it?

I'd expect the router to be provided only via router configuration and not exposed anywhere else.

Sorry, something went wrong.

Uh oh!

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

All reactions

@jasonaden jasonaden Apr 11, 2018

Copy link
Copy Markdown
Contributor

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.

Good point. We should be able to take it off the router class.

Sorry, something went wrong.

Uh oh!

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

All reactions

@jasonaden jasonaden Apr 11, 2018

Copy link
Copy Markdown
Contributor

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.

Marking as @internal and inlining in ExtraOptions after discussing with Igor.

Sorry, something went wrong.

Uh oh!

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

All reactions
@wKoza

wKoza commented Apr 11, 2018

Copy link
Copy Markdown
Contributor

@jasonaden , I don't understand why we should have a url parameter on the type MalformedUriErrorHandler. We say:

'url' - url redirect to in a case of an error.

But, it's wrong. We should say:

'url' - default url redirect to provided by Angular in a case of an error

url does not have much interest, it is always valued to \:

} catch (e) {
      urlTree = this.malformedUriErrorHandler(e, this.urlSerializer, '/');
}
All reactions

Sorry, something went wrong.

Uh oh!

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

@IgorMinar IgorMinar added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: merge The PR is ready for merge by the caretaker labels Apr 11, 2018
@jasonaden

jasonaden commented Apr 11, 2018

Copy link
Copy Markdown
Contributor

@wKoza So the reason for passing the url here is that it's the url that failed to parse. Maybe as an application developer you want to recover in a different way than just going to /. Maybe you want to detect things such as non-encoded % sign, determine it's not part of a percent encoding set, and encode it manually. For example:

Incoming URL that will fail: http://mysite.com/foo%bar

Possible recovery URL: http://mysite.com/foo%25bar

You can only make this type of recovery if we pass the offending URL to the error handler.

All reactions

Sorry, something went wrong.

Uh oh!

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

@wKoza

wKoza commented Apr 13, 2018

Copy link
Copy Markdown
Contributor

thanks @jasonaden. Indeed, that's what we can expect. But we should not do this instead:

parseUrl(url: string): UrlTree {
    let urlTree: UrlTree;
    try {
      urlTree = this.urlSerializer.parse(url);
    } catch (e) {
      urlTree = this.malformedUriErrorHandler(e, this.urlSerializer, url);
    }
    return urlTree;
 }

If we want to catch the url ?

All reactions

Sorry, something went wrong.

Uh oh!

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

@IgorMinar IgorMinar modified the milestones: v6.0, v6-candidates Apr 16, 2018
@jasonaden

jasonaden commented Apr 17, 2018

Copy link
Copy Markdown
Contributor

@wKoza Yes, you're right. My push back to this branch appears to have gotten blocked last week, but see the code now. It's what you have noted above, plus the changes @IgorMinar wanted.

All reactions

Sorry, something went wrong.

Uh oh!

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

wKoza
wKoza reviewed Apr 17, 2018
View reviewed changes
Comment thread packages/router/src/router_module.ts Outdated

@wKoza wKoza Apr 17, 2018

Copy link
Copy Markdown
Contributor

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.

@jasonaden , urldefinition should be Url causing the error

Sorry, something went wrong.

Uh oh!

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

All reactions
@jasonaden
jasonaden force-pushed the fix-malformed-url-error branch from 04b2330 to 2bbf206 Compare April 18, 2018 22:10
sergeome @jasonaden
fix(router): add ability to recover from malformed url
a04a1f8
Fixes angular#21468
@jasonaden
jasonaden force-pushed the fix-malformed-url-error branch from 2bbf206 to a04a1f8 Compare April 18, 2018 22:45
@jasonaden
fixup! fix(router): add ability to recover from malformed url
630b1f3
@jasonaden

jasonaden commented Apr 19, 2018

Copy link
Copy Markdown
Contributor

I couldn't mark Router.malformedUriErrorHandler as @internal because it needs to be used in the RouterTestingModule. So it's a public property, but the type info is inlined so it will behave correctly in documentation.

All reactions

Sorry, something went wrong.

Uh oh!

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

@jasonaden jasonaden added the target: patch This PR is targeted for the next patch release label Apr 20, 2018
@wbhob

wbhob commented Jul 6, 2018

Copy link
Copy Markdown

@sergeome what's the status of this PR? Looks like it hasn't been touched for a few months. CodeTriage sent me here to check in.

All reactions

Sorry, something went wrong.

Uh oh!

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

@skryvets

skryvets commented Jul 6, 2018

Copy link
Copy Markdown
Author

@wbhob, as far as I understand there some internal discussions should happen within the Angular team.
@jasonaden probably can speak to that.
Please, correct me if I'm wrong.

All reactions

Sorry, something went wrong.

Uh oh!

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

@jasonaden
jasonaden dismissed IgorMinar’s stale review July 10, 2018 23:00

Review comments addressed

@jasonaden jasonaden added action: merge The PR is ready for merge by the caretaker and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Jul 10, 2018
@jasonaden jasonaden changed the title fix(router): add ability to recover from malformed url feat(router): add ability to recover from malformed url Jul 10, 2018
mhevery
mhevery approved these changes Jul 11, 2018
View reviewed changes
@mhevery mhevery closed this in 86d254d Jul 11, 2018
mhevery pushed a commit that referenced this pull request Jul 11, 2018
sergeome @mhevery
fix(router): add ability to recover from malformed url (#23283)
2d4f4b5
Fixes #21468

PR Close #23283
danielsogl added a commit to danielsogl/angular that referenced this pull request Jul 25, 2018
@danielsogl
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
@danielsogl @IgorMinar
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
@angular-automatic-lock-bot

angular-automatic-lock-bot Bot commented Sep 13, 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 angular-automatic-lock-bot Bot locked and limited conversation to collaborators Sep 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

4 more reviewers
@jasonaden jasonaden jasonaden approved these changes
@wKoza wKoza wKoza left review comments
@mhevery mhevery mhevery approved these changes
@IgorMinar IgorMinar IgorMinar left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

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

Assignees

No one assigned

Labels

action: merge The PR is ready for merge by the caretaker area: router cla: yes target: patch This PR is targeted for the next patch release type: bug/fix

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.

7 participants

@skryvets @wKoza @jasonaden @wbhob @mhevery @IgorMinar @googlebot
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.