Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

perf(core): Make `PlatformLocation` tree-shakable #32154

Closed
wants to merge 3 commits into from

Conversation

@mhevery
Copy link
Member

commented Aug 15, 2019

Convert PlatformLocation into a tree-shakable provider

@googlebot googlebot added the cla: yes label Aug 15, 2019

@mhevery mhevery force-pushed the mhevery:location_cleanup branch 3 times, most recently from 2fc9b4c to e0dac02 Aug 15, 2019

@mhevery mhevery force-pushed the mhevery:location_cleanup branch 10 times, most recently from 36286e9 to 18c90f9 Aug 15, 2019

@mhevery mhevery marked this pull request as ready for review Aug 22, 2019

@mhevery mhevery requested review from angular/fw-compiler as code owners Aug 22, 2019

@mhevery mhevery force-pushed the mhevery:location_cleanup branch from 049bce4 to e3dce40 Aug 22, 2019

@mhevery mhevery requested a review from angular/fw-integration as a code owner Aug 22, 2019

@alxhub
alxhub approved these changes Aug 22, 2019
Copy link
Contributor

left a comment

Overall this LGTM.

The only thing I would suggest is this PR is one commit without much of a description, and it does a lot more than it says in the commit title. I would recommend breaking the PR into 2-3 commits (esp. one for the change to tree-shakeable provider infrastructure which allows platform providers). At the very least the commit description should document what changes were made to the overall infrastructure to allow the conversion of PlatformLocation to be tree-shakeable.

@@ -14,5 +14,4 @@ import {InjectionToken} from './injection_token';
* as a root scoped injector when processing requests for unknown tokens which may indicate
* they are provided in the root scope.
*/
export const APP_ROOT = new InjectionToken<boolean>(
'The presence of this token marks an injector as being the root injector.');
export const INJECTOR_SCOPE = new InjectionToken<'root'|'platform'|null>('Set Injector scope.');

This comment has been minimized.

Copy link
@alxhub

alxhub Aug 22, 2019

Contributor

Just FYI, I believe there may be some projects in g3 which make use of this private API.

This comment has been minimized.

Copy link
@mhevery

mhevery Aug 23, 2019

Author Member

thanks for the heads up

packages/core/src/di/r3_injector.ts Outdated Show resolved Hide resolved
packages/core/src/di/r3_injector.ts Show resolved Hide resolved
packages/platform-server/src/server.ts Outdated Show resolved Hide resolved
@mhevery

This comment has been minimized.

Copy link
Member Author

commented Aug 23, 2019

@alxhub Thanks for the feedback. I have broken the PR into 3 SHAs.

@mhevery mhevery force-pushed the mhevery:location_cleanup branch from e3dce40 to d305674 Aug 23, 2019

@mhevery

This comment has been minimized.

Copy link
Member Author

commented Aug 29, 2019

@mhevery mhevery force-pushed the mhevery:location_cleanup branch from 63fbd8e to 7d147a7 Aug 29, 2019

@mhevery

This comment has been minimized.

Copy link
Member Author

commented Aug 29, 2019

@mhevery mhevery force-pushed the mhevery:location_cleanup branch 2 times, most recently from c5b76ae to e323e19 Aug 29, 2019

mhevery added 2 commits Aug 23, 2019
refactor: Move `dom_adapter.ts` to `@angular/common`
This work is needed in preparation for turning tokens into tree-shakable injectables.
feat(core): Adds DI support for `providedIn: 'platform'|'any'`
Extend the vocabulary of the `providedIn` to also include  `'platform'` and `'any'`` scope.
```
@Injectable({
  providedId: 'platform', // tree shakable injector for platform injector
})
class MyService {...}
```

@mhevery mhevery force-pushed the mhevery:location_cleanup branch 2 times, most recently from 0edcbeb to 370f6d9 Aug 29, 2019

@mhevery

This comment has been minimized.

Copy link
Member Author

commented Aug 29, 2019

perf(core): Make `PlatformLocation` tree-shakable
Convert `PlatformLocation` into a tree-shakable provider.

@mhevery mhevery force-pushed the mhevery:location_cleanup branch from 370f6d9 to 613877a Aug 29, 2019

@mhevery

This comment has been minimized.

Copy link
Member Author

commented Aug 29, 2019

@mhevery

This comment has been minimized.

Copy link
Member Author

commented Aug 30, 2019

MERGE ASSISTANCE: needs cl/266062693 patch

@mhevery mhevery closed this in 8a47b48 Aug 30, 2019

mhevery added a commit that referenced this pull request Aug 30, 2019
feat(core): Adds DI support for `providedIn: 'platform'|'any'` (#32154)
Extend the vocabulary of the `providedIn` to also include  `'platform'` and `'any'`` scope.
```
@Injectable({
  providedId: 'platform', // tree shakable injector for platform injector
})
class MyService {...}
```

PR Close #32154
mhevery added a commit that referenced this pull request Aug 30, 2019
perf(core): Make `PlatformLocation` tree-shakable (#32154)
Convert `PlatformLocation` into a tree-shakable provider.

PR Close #32154
cexbrayat added a commit to cexbrayat/angular that referenced this pull request Aug 30, 2019
style(core): typos in docs and tests
PR angular#32154 introduced `platform` and `any` for `providedIn` and the doc has a minor typo.
Also a test name was not changed accordingly to the refactoring done.
@cexbrayat cexbrayat referenced this pull request Aug 30, 2019
3 of 14 tasks complete
@razvanm

This comment has been minimized.

Copy link

commented Sep 4, 2019

As far as I can tell this PR broke the overriding of Location.stripTrailingSlash mentioned in #16051 (comment). Is that expected?

mhevery added a commit that referenced this pull request Sep 4, 2019
style(core): typos in docs and tests (#32410)
PR #32154 introduced `platform` and `any` for `providedIn` and the doc has a minor typo.
Also a test name was not changed accordingly to the refactoring done.

PR Close #32410
@mhevery

This comment has been minimized.

Copy link
Member Author

commented Sep 5, 2019

@razvanm What specifically broke. We just moved code around, there should be no changes in behavior.

@razvanm

This comment has been minimized.

Copy link

commented Sep 5, 2019

In the code I'm working with I had in my root app the following code:

Location.stripTrailingSlash = (url: string) => {
  return url.endsWith('/') && url.length > 1 ? url + '.' : url;
};

This used to work fine both in dev mode and in the production code that gets compiled using the Closure compiler. After this PR the overriding stopped working (I used some console.log() calls to figure out that the Angular code is called instead of mine).

@mhevery

This comment has been minimized.

Copy link
Member Author

commented Sep 5, 2019

sabeersulaiman added a commit to sabeersulaiman/angular that referenced this pull request Sep 6, 2019
refactor: Move `dom_adapter.ts` to `@angular/common` (angular#32154)
This work is needed in preparation for turning tokens into tree-shakable injectables.

PR Close angular#32154
sabeersulaiman added a commit to sabeersulaiman/angular that referenced this pull request Sep 6, 2019
feat(core): Adds DI support for `providedIn: 'platform'|'any'` (angul…
…ar#32154)

Extend the vocabulary of the `providedIn` to also include  `'platform'` and `'any'`` scope.
```
@Injectable({
  providedId: 'platform', // tree shakable injector for platform injector
})
class MyService {...}
```

PR Close angular#32154
sabeersulaiman added a commit to sabeersulaiman/angular that referenced this pull request Sep 6, 2019
perf(core): Make `PlatformLocation` tree-shakable (angular#32154)
Convert `PlatformLocation` into a tree-shakable provider.

PR Close angular#32154
sabeersulaiman added a commit to sabeersulaiman/angular that referenced this pull request Sep 6, 2019
style(core): typos in docs and tests (angular#32410)
PR angular#32154 introduced `platform` and `any` for `providedIn` and the doc has a minor typo.
Also a test name was not changed accordingly to the refactoring done.

PR Close angular#32410
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.