Skip to content

feat(platform-browser) add LinkDefinition and Link service#34606

Closed
d-koppenhagen wants to merge 10 commits into
angular:masterfrom
d-koppenhagen:feat/linkService
Closed

feat(platform-browser) add LinkDefinition and Link service#34606
d-koppenhagen wants to merge 10 commits into
angular:masterfrom
d-koppenhagen:feat/linkService

Conversation

@d-koppenhagen
Copy link
Copy Markdown
Contributor

@d-koppenhagen d-koppenhagen commented Jan 1, 2020

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • 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?

Issue Number: #34605 (Provide a LinkService)

What is the new behavior?

Not implemented yet.

Does this PR introduce a breaking change?

  • Yes
  • No

@d-koppenhagen d-koppenhagen requested a review from a team January 1, 2020 10:28
@d-koppenhagen d-koppenhagen changed the title WIP: Implement Link service and LinkDefinition Implement Link service and LinkDefinition Jan 1, 2020
@d-koppenhagen d-koppenhagen changed the title Implement Link service and LinkDefinition feat(platform-browser) add LinkDefinition and Link service and `` Jan 2, 2020
@d-koppenhagen d-koppenhagen changed the title feat(platform-browser) add LinkDefinition and Link service and `` feat(platform-browser) add LinkDefinition and Link service Jan 2, 2020
@d-koppenhagen
Copy link
Copy Markdown
Contributor Author

d-koppenhagen commented Jan 3, 2020

My circle CI tests are failing because of the following error:

//tools/public_api_guard:upgrade_api                                     PASSED in 12.5s
//tools/public_api_guard:upgrade_static_api                              PASSED in 14.6s
//tools/public_api_guard:platform-browser_api                            FAILED in 2 out of 2 in 11.0s
  Stats over 2 runs: max = 11.0s, min = 8.5s, avg = 9.8s, dev = 1.2s
  /home/circleci/.cache/bazel/_bazel_circleci/9ce5c2144ecf75d11717c0aa41e45a8d/execroot/angular/bazel-out/k8-fastbuild/testlogs/tools/public_api_guard/platform-browser_api/test.log
  /home/circleci/.cache/bazel/_bazel_circleci/9ce5c2144ecf75d11717c0aa41e45a8d/execroot/angular/bazel-out/k8-fastbuild/testlogs/tools/public_api_guard/platform-browser_api/test_attempts/attempt_1.log
//packages/core/test:test                                                PASSED in 15.4s
  Stats over 4 runs: max = 15.4s, min = 12.0s, avg = 14.2s, dev = 1.3s

Executed 125 out of 200 tests: 199 tests pass and 1 fails remotely.
There were tests whose specified size is too big. Use the --test_verbose_timeout_warnings command line option to see which ones these are.
error Command failed with exit code 3.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

Exited with code exit status 3

Any clue how to fix this?

@atscott atscott added the area: core Issues related to the framework runtime label Jan 9, 2020
@ngbot ngbot Bot added this to the needsTriage milestone Jan 9, 2020
@d-koppenhagen d-koppenhagen requested review from a team January 15, 2020 06:34
@d-koppenhagen d-koppenhagen requested a review from a team January 22, 2020 18:47
@d-koppenhagen d-koppenhagen mentioned this pull request Jan 24, 2020
Open
@pullapprove pullapprove Bot requested a review from AndrewKushnir January 29, 2020 20:25
@pullapprove pullapprove Bot requested a review from IgorMinar February 6, 2020 23:33
@mhevery
Copy link
Copy Markdown
Contributor

mhevery commented Feb 26, 2020

Any clue how to fix this?

run yarn bazel run //tools/public_api_guard:platform-browser_api.accept

mhevery
mhevery suggested changes Feb 26, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@mhevery mhevery left a comment

Choose a reason for hiding this comment

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

This seems to be a new feature. Please add:

  • Description of a problem this is solving.
  • Developer documentation so that someone can discover it and use it.

Without the above we don't really know what we are reviewing and it can't be merged since the developers will not know how to use it.

@d-koppenhagen
feat(platform-browser): implemented LinkDefinition and Link Service
9c69fe3 
Closes angular#34605
@d-koppenhagen
style(platform-browser): format code
c7aab70
@d-koppenhagen
style(platform-browser): reformat code and tests
57731e6
@d-koppenhagen
test(platform-browser): fix tests for Link service
ff7a2d0
@d-koppenhagen
fix(platform-browser): fix _parseSelector implementation
c22c674
@d-koppenhagen
test(platform-browser): replace 'meta' with 'link' in test
48984cf
@d-koppenhagen
refactor(platform-browser): update default LinkDefinition props
4400de4 
see: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/link
@d-koppenhagen
fix(platform-browser): add missing Link and LinkDefinition to .d.ts
5c3e11e
@d-koppenhagen
fix(platform-browser): remove .angulardoc.json file
55fe8e3
@d-koppenhagen
fix: accept public api change
ac43446 
...by running `yarn bazel run //tools/public_api_guard:platform-browser_api.accept`
@d-koppenhagen
Copy link
Copy Markdown
Contributor Author

d-koppenhagen commented Feb 28, 2020 •
edited
Loading

@mhevery thanks for the reply. As it's my first PR, I may not that experiences in what to add at which place.

Description of a problem this is solving.

  • Where do I need to add this description? Just here in the PR?

Developer documentation so that someone can discover it and use it.

  • Should this be a new markdown file here /aio/content/guide? Or somewhere else?

@d-koppenhagen
Copy link
Copy Markdown
Contributor Author

d-koppenhagen commented Feb 28, 2020

The Link service with it's interface LinkDefinition is a service to add and modify HTML <link> elements in the <head>of the application.
It will provide an API that makes modifying link data easier without touching the window / dom directly by a developer using Angular (similar to the existing Meta service).

One example for using the service is to add canonical URLs dynamically to your app.

Without the service the implementation for doing this would look like this for an angular developer:

import { Injectable, Inject } from '@angular/core';
import { DOCUMENT } from '@angular/common';

@Injectable({
  providedIn: 'root',
})
export class LinkService {
  constructor(@Inject(DOCUMENT) private dom) {}

  createCanonicalURL(url: string) {
    let link: HTMLLinkElement = document.querySelector('link[rel="canonical"]');
    if (link) {
      link.setAttribute('href', url);
    } else {
      link = this.dom.createElement('link');
      link.setAttribute('rel', 'canonical');
      this.dom.head.appendChild(link);
      link.setAttribute('href', url);
    }
  }
}

After the public Link service class is available, the implementation is much nicer:

import { Injectable, Inject } from '@angular/core';
import { Link } from '@angular/platform-browser';

@Injectable({
  providedIn: 'root',
})
export class LinkService {
  constructor(public link: Meta,) {}

  createCanonicalURL(url: string) {
    this.link.updateTag({ rel: 'canonical', href: url });
  }
}

@petebacondarwin petebacondarwin added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Apr 15, 2020
@petebacondarwin petebacondarwin removed request for a team April 15, 2020 09:48
@mhevery
Copy link
Copy Markdown
Contributor

mhevery commented Nov 17, 2020

Hello, after discussing with the team, we don't think we want to be adding new API surface to the Angular at this time. I will close it for now.

@mhevery mhevery closed this Nov 17, 2020
@angular-automatic-lock-bot
Copy link
Copy Markdown

angular-automatic-lock-bot Bot commented Dec 19, 2020

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 Dec 19, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@AndrewKushnir AndrewKushnir Awaiting requested review from AndrewKushnir

@IgorMinar IgorMinar Awaiting requested review from IgorMinar

1 more reviewer

@mhevery mhevery mhevery requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

@mhevery mhevery

Labels

action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews area: core Issues related to the framework runtime cla: yes

Projects

None yet

Milestone

needsTriage

Development

Successfully merging this pull request may close these issues.

5 participants

@d-koppenhagen @mhevery @petebacondarwin @atscott @googlebot