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

refactor(Location): out of router and into platform/common #7962

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
7 participants
@NathanWalker
Contributor

NathanWalker commented Apr 8, 2016

  • update documentation to reflect changes
  • discuss barrels/decide final placement for several auxiliary location classes.
  • update public_api_spec.ts with new api changes
  • squash and update commit message to reflect breaking changes

@mhevery I don't want this to affect the hard work going into making the ~ 10 KB goal for core.

I could foresee Location, LocationStrategy, HashLocationStrategy, PathLocationStrategy, APP_BASE_HREF going into angular2/common.

Please review when possible and let me know your thoughts.

@googlebot googlebot added the cla: yes label Apr 8, 2016

@NathanWalker NathanWalker changed the title from refactor(Location): out of router and into core to refactor(Location): out of router and into core (or common) Apr 8, 2016

@NathanWalker

This comment has been minimized.

Contributor

NathanWalker commented Apr 8, 2016

Ok. I just added 1 more commit that provides a 2nd alternative refactoring of Location into common.
You can now compare both 1st and 2nd commits to see both refactor paths.

After working with the first refactor pass into core (the 1st commit), it occurred to me that a Location service (IMO) is perhaps, not a core feature. There are many applications that never need a Location service, however it is common.

For this reason, I wanted to provide an alternative refactor which instead wraps Location into angular2/common. This is also advantageous as it does not affect core. With the 2nd commit, the breaking changes would be:

BEFORE:

import {
  PlatformLocation,
  Location,
  LocationStrategy,
  HashLocationStrategy,
  PathLocationStrategy,
  APP_BASE_HREF}
from 'angular2/router';

AFTER:

import {PlatformLocation} from 'angular2/src/facade/platform_location';
import {
  Location,
  LocationStrategy,
  HashLocationStrategy,
  PathLocationStrategy,
  APP_BASE_HREF}
from 'angular2/common';

Since PlatformLocation should not be used directly by a common application developer (more for advanced usage), I believe this is well served in facade and not provided by a barrel.

Please let me know which path you like better and I'll squash/add breaking changes to commit message, then update the documentation throughout the files to appropriately reflect the changes as well as update public_api_spec.ts per your approval.

CI breaking is due only to public_api_spec.ts regarding api changes here.

@gdi2290

This comment has been minimized.

Member

gdi2290 commented Apr 9, 2016

you will probably have to wait for this to be merged #7984

@gdi2290

This comment has been minimized.

Member

gdi2290 commented Apr 9, 2016

we also need to merge History class before refactoring the router location. At the moment, the current router Location handles both Location and History

@NathanWalker

This comment has been minimized.

Contributor

NathanWalker commented Apr 9, 2016

Thanks for heads up @gdi2290, is there an issue or PR for merging History class?
I could create one?

Also I would greatly appreciate your feedback on this PR.
Main goal is to make Location service available without including the router package.
I'm open to any refactor paths to achieve that; the 2 here are just a way forward that appears clean and makes sense to me.

@mhevery mhevery self-assigned this Apr 9, 2016

@mhevery

This comment has been minimized.

Member

mhevery commented Apr 9, 2016

Hi @NathanWalker, thanks for your PR. I don't think that Location should be part of the core since there could be platforms which do not have location concept. Also different platform may have different shape of Location, for example stack or a tree, hence it is hard to generalize.

I could see how you may want to share the Location between Browser and Server but I don't think it should be part of the core. I would be happy to hear arguments to the contrary.

@NathanWalker

This comment has been minimized.

Contributor

NathanWalker commented Apr 9, 2016

Agreed. On my 2nd commit I show an alternate refactor path placing it in angular2/common instead, however I assume that still ends up being bundled in angular2.dev.js?

Would it make sense to have a separate barrel for this, perhaps angular2/location which would get bundled to 'angular2/bundles/location.js'?

Just trying to create a path forward for those trying to upgrade from Angular 1.x which provided $location as a core service. My company's large Angular 1.x app for instance has an unauthenticated portion of our app which does not use the router however uses $location extensively for various marketing reasons. Therefore, we are in process of upgrading to 2.0 and facing the scenario of having to include the router bundle of 76 KB in those unauthenticated areas just to get access to the Location service :(

@mhevery

This comment has been minimized.

Member

mhevery commented Apr 9, 2016

I think Location should be moved to src/platform/browser or src/platform/common. If you could move it there I would be fine with it. Could you update the PR and squash it?

@NathanWalker

This comment has been minimized.

Contributor

NathanWalker commented Apr 10, 2016

I moved it into src/platform/browser. I like it; makes a lot of sense to me.
Squashed commit and added breaking change block to message.

I'll update the documentation throughout the src by Monday/Tuesday. In meantime, when you can, please review these changes and let me know if you'd like anything else changed. Thanks for your attention to this.

@NathanWalker NathanWalker changed the title from refactor(Location): out of router and into core (or common) to refactor(Location): out of router and into platform/browser Apr 10, 2016

@@ -1,8 +1,8 @@
import {Directive} from 'angular2/core';
import {Location} from 'angular2/src/platform/browser/location';

This comment has been minimized.

@mhevery

mhevery Apr 10, 2016

Member

In general one should never import anything between libraries which includes src in the URL. You can think of Angular2 and Router as separate libraries.

The correct import should be from angular2/platform/... Which means that the Location and friends need to be re-exported from there. Think of src as private and angular2/platform as public. Library can only import from another library public API.

This is a general issue I have seen in many imports. Please canonicalize them.

'Title',
'bootstrap',
'disableDebugTools',
'enableDebugTools',
'inspectNativeElement'
'inspectNativeElement',
'joinWithSlash',

This comment has been minimized.

@mhevery

mhevery Apr 10, 2016

Member

'joinWithSlashandnormilzeQueryParamsshould be a static member on existing type such asLocation`. I don't think they should be top level imports.

@@ -17,10 +17,10 @@ import {
import {SpyRouter, SpyLocation} from '../spies';
import {provide, Component} from 'angular2/core';
import {Location} from 'angular2/platform/browser';

This comment has been minimized.

@mhevery

mhevery Apr 10, 2016

Member

correct import (as an example)

@mhevery

This comment has been minimized.

Member

mhevery commented Apr 10, 2016

Other than inconsistent import's this looks good to me. Please ping when ready so that we can merge it.

@NathanWalker NathanWalker changed the title from refactor(Location): out of router and into platform/browser to refactor(Location): out of router and into platform/common Apr 11, 2016

'APP_BASE_HREF', 'HashLocationStrategy', 'Location', 'LocationStrategy', 'PathLocationStrategy',
'PlatformLocation'
];

This comment has been minimized.

@NathanWalker

NathanWalker Apr 11, 2016

Contributor

May not be necessary but I thought you may want to have an API check for this to help with further testing if more is added to platform/common in the future.

@NathanWalker

This comment has been minimized.

Contributor

NathanWalker commented Apr 11, 2016

Ok due to fact that many tests can be run outside the browser, I ended up having to go with your other suggestion platform/common which works out the best.

Using platform/browser was problematic as it would execute imports trailing down to angular2/src/platform/browser/tools/common_tools which contains:

import {window} from 'angular2/src/facade/browser';

And this was not good for platform agnostic configurations and tests, etc. I thought about placing them in platform/common_dom however I think that would probably cause confusion down the line as they are not DOM specific.

platform/common was a great suggestion you had and this also provides a place for other platform agnostic services to grow into the future.

Please have a look when you can and hopefully this still meets your approval.

@NathanWalker

This comment has been minimized.

Contributor

NathanWalker commented Apr 12, 2016

Just for ref (mentioned in commit msg):

Location and other related providers have been moved out of router and into platform/common. BrowserPlatformLocation is not meant to be used directly however advanced configurations may use it via the following import change.

Before:

import {
  PlatformLocation,
  Location,
  LocationStrategy,
  HashLocationStrategy,
  PathLocationStrategy,
  APP_BASE_HREF}
from 'angular2/router';

import {BrowserPlatformLocation} from 'angular2/src/router/location/browser_platform_location';

After:

import {
  PlatformLocation,
  Location,
  LocationStrategy,
  HashLocationStrategy,
  PathLocationStrategy,
  APP_BASE_HREF}
from 'angular2/platform/common';

import {BrowserPlatformLocation} from 'angular2/src/platform/browser/location/browser_platform_location';

@gdi2290 gdi2290 referenced this pull request Apr 12, 2016

Open

Drop DomAdapter #7561

@mhevery

This comment has been minimized.

Member

mhevery commented Apr 12, 2016

Looking good. You have a broken test:

    Expected [ '-UrlChangeEvent', '-UrlChangeListener' ] to equal [  ].
    main/</</<@/home/travis/build/angular/angular/dist/js/dev/es5/angular2/test/public_api_spec.js:200:17 <- diffing_plugin_wrapper-output_path-V43qaiXg.tmp/angular2/test/public_api_spec.ts:213:8
    _it/<@/home/travis/build/angular/angular/dist/js/dev/es5/angular2/src/testing/testing_internal.js:170:17 <- diffing_plugin_wrapper-output_path-V43qaiXg.tmp/angular2/src/testing/testing_internal.ts:171:21

Can you have a look at it? Otherwise this is good to go.

@NathanWalker

This comment has been minimized.

Contributor

NathanWalker commented Apr 13, 2016

Whenever I try to fix it for Dart it breaks for JS and vice versa.
Fixing the above error for JS (in saucelabs) then creates this Dart error which is the opposite:

[19:45:52] Starting 'test.unit.dart/ci'...
[19:45:52] Starting 'test.dart.dartium_symlink'...
[19:45:52] Finished 'test.dart.dartium_symlink' after 23 ms
[19:45:52] Starting '!test.unit.dart/run/angular2'...
...
00:56 +1952 ~4 -1: public API should fail if public API for ngPlatformCommon has changed           
  Expected: has different properties
    Actual: ['+UrlChangeEvent', '+UrlChangeListener']
     Which: is equal to [+UrlChangeEvent, +UrlChangeListener]. Expected: []

It appears Dart expects them whereas JS (saucelabs) does not:

[00:46:11] Starting 'test.unit.js.sauce/ci'...
...
Firefox 42.0.0 (Linux 0.0.0) public API should fail if public API for ngPlatformCommon has changed FAILED
    Expected [ '-UrlChangeEvent', '-UrlChangeListener' ] to equal [  ].
    main/</</<@/home/travis/build/angular/angular/dist/js/dev/es5/angular2/test/public_api_spec.js:200:17

Am I missing something? Maybe a separate Dart file I'm not seeing?

@mhevery

This comment has been minimized.

Member

mhevery commented Apr 13, 2016

I think because UrlChangeEvent is an interface it only shows up in Dart. Adding UrlChangeEvent:dart should fix the problem.

@NathanWalker

This comment has been minimized.

Contributor

NathanWalker commented Apr 13, 2016

You're brilliant @mhevery. Don't let nobody tell ya' different.
That got it, thanks!

refactor(Location): out of router and into platform/common
closes #4943

BREAKING CHANGE:

`Location` and other related providers have been moved out of `router` and into `platform/common`. `BrowserPlatformLocation` is not meant to be used directly however advanced configurations may use it via the following import change.

Before:

```
import {
  PlatformLocation,
  Location,
  LocationStrategy,
  HashLocationStrategy,
  PathLocationStrategy,
  APP_BASE_HREF}
from 'angular2/router';

import {BrowserPlatformLocation} from 'angular2/src/router/location/browser_platform_location';
```

After:

```
import {
  PlatformLocation,
  Location,
  LocationStrategy,
  HashLocationStrategy,
  PathLocationStrategy,
  APP_BASE_HREF}
from 'angular2/platform/common';

import {BrowserPlatformLocation} from 'angular2/src/platform/browser/location/browser_platform_location';
```
@mhevery

This comment has been minimized.

Member

mhevery commented Apr 15, 2016

Thank you. Added the right labels to get it merged...

@mary-poppins

This comment has been minimized.

mary-poppins commented Apr 20, 2016

Merging PR #7962 on behalf of @robertmesserle to branch presubmit-robertmesserle-pr-7962.

@Delagen

This comment has been minimized.

Contributor

Delagen commented Apr 26, 2016

+1 #8240

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment