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

feat: Abstract tokens #7984

Closed
wants to merge 3 commits into from
Closed

Conversation

PatrickJS
Copy link
Member

  • Please check if the PR fulfills these requirements
  • The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit-message-format
  • What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
    feature
  • What is the current behavior? (You can also link to an open issue here)
    Every 3rd party module needs to create their own Location service (e.g. the component router)
  • What is the new behavior (if this is a feature change)?
    Creates a better contract for Universal code, 3rd party modules, and testing similar to $window in Angular 1
  • Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
    Yes, Previously the Title service was exported
  • Other information:
    cc @jeffbcross

BREAKING CHANGE:
Previously Title was exported. Now Title has been renamed to TitleImpl
/**
* An interface to get the Global object
*/
export abstract class Global {
Copy link
Member Author

Choose a reason for hiding this comment

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

perhaps window is a better name after all when considering window as the context of where the app is run (iFrame, node context, browser, web worker)
#7979 (comment)
GlobalContext, Window, or maybe even Context may also work

@jeffbcross jeffbcross self-assigned this Apr 8, 2016
fix(platform/tokens): correct export Types

fix(platform/browser_common): _location() use getLocation()

style(platform/tokens): remove abstract for props

test(title): inject Title

refactor(public_api): remove Title

style(Title): clang format
@mhevery
Copy link
Contributor

mhevery commented Apr 9, 2016

Could you explain the reason why the Title was hidden? It seems like something which should be public.

@PatrickJS
Copy link
Member Author

@mhevery If you're referring to the Title abstract class then the current path is import {Title} from 'angular2/platform/tokens'; which can be exported in core.ts to make it be public with angular2/core. If you meant TitleImpl (maybe it should be named Title_ or BrowserTitle) then for the same reason XHRImpl(angular2/src/platform/browser/xhr_impl) and MessageBasedXHRImpl (angular2/src/web_workers/ui/xhr_impl) are not public. A developer can use them if import with a direct path otherwise the abstract classes will work. I should probably test that Title is an instance of TitleImpl though

@@ -3,12 +3,12 @@ import {DOM} from 'angular2/src/platform/dom/dom_adapter';
/**
* A service that can be used to get and set the title of a current HTML document.
*
* Since an Angular 2 application can't be bootstrapped on the entire HTML document (`<html>` tag)
* When an Angular 2 application is not bootstrapped on the entire HTML document (`<html>` tag)
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this mean you actually can do that? I mean, sure you can, but that purges the entire <head> and I wouldn't assume someone would actually want that. This text sounds to me like bootstrapping Angular to <html> is the common case and for edge cases the Title service can be used.
Just my 2c

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is right. Even if I bootstrap part of the DOM I should still be able to access the title. I think a better way to think about it is that the Title should be part of the platform injector, since there is only one injector per page.

Alternative way to think about it is that Title is owned by each application. When you write to the title you clober whatever there was before. (ie, if multiple applications bootstrap on a single page then the last write to the API wins.)

@mhevery
Copy link
Contributor

mhevery commented Apr 9, 2016

OK, I thought about it some more, and I am not convinced that the the Title should be moved from the Browser to the platform. The issue is that I can imagine a platform which does not have Title or even a platform where the Title is more complicated than just a simple string.

I am leaning against this refactoring. Please provide reasons why you think this should move?

@mhevery
Copy link
Contributor

mhevery commented May 20, 2016

no new activity, closing

@mhevery mhevery closed this May 20, 2016
@PatrickJS
Copy link
Member Author

moving Location to common is a better location for what I was intending. These (Title, Location, Global) should be abstract classes in @angular/common and Global should be Window which is the context of the application enviroment which could be different from Node's global context

@PatrickJS PatrickJS deleted the abstract-tokens branch May 20, 2016 03:59
@angular-automatic-lock-bot
Copy link

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants