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(compiler): deprecate i18n comments in favor of `ng-container` #18998

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
5 participants
@ocombe
Copy link
Contributor

commented Sep 1, 2017

PR Type

What kind of change does this PR introduce?

[x] Feature

What is the current behavior?

I18n comments require a lot of code to work, it's a big effort to maintain and it can easily be replaced by <ng-container i18n></ng-container>

What is the new behavior?

I18n comments have been deprecated and will print a warning in dev mode

Does this PR introduce a breaking change?

[x] No
@@ -158,6 +171,9 @@ class _Visitor implements html.Visitor {
return icu;
}

/**
* @deprecated from v5 you should use <ng-container i18n> instead of i18n comments

This comment has been minimized.

Copy link
@ocombe

ocombe Sep 1, 2017

Author Contributor

I wasn't sure where to put the deprecated tag, I guess here is as good as any

This comment has been minimized.

Copy link
@vicb

vicb Sep 1, 2017

Contributor

visitComment is not deprecated.
Remove @deprecated on the method add a // deprecated comment in the code

@mary-poppins

This comment has been minimized.

Copy link

commented Sep 1, 2017

import * as html from '../ml_parser/ast';
import {InterpolationConfig} from '../ml_parser/interpolation_config';
import {ParseTreeResult} from '../ml_parser/parser';

import {ParseErrorLevel} from '../parse_util';
import {warnOnlyOnce} from '../template_parser/template_parser';

This comment has been minimized.

Copy link
@vicb

vicb Sep 1, 2017

Contributor

no.
either refactor it to have it at a sensible place or copy it.
I have a preference for the later until we have a better logging/deprecation story. And yes I would love for someone to work on it.

@@ -119,7 +123,16 @@ class _Visitor implements html.Visitor {
this._reportError(nodes[nodes.length - 1], 'Unclosed block');
}

return new ParseTreeResult(translatedNode.children, this._errors);
const warnings = this._errors !.filter(error => error.level === ParseErrorLevel.WARNING)
.filter(warnOnlyOnce([I18N_COMMENTS_WARNING]));

This comment has been minimized.

Copy link
@vicb

vicb Sep 1, 2017

Contributor

Why would warnings always be i18n comments ?

@@ -176,6 +192,10 @@ class _Visitor implements html.Visitor {
if (!this._inI18nNode && !this._inIcu) {
if (!this._inI18nBlock) {
if (isOpening) {
if (isDevMode()) {

This comment has been minimized.

Copy link
@vicb

vicb Sep 1, 2017

Contributor

check if it is the first time we hit the if clause and warn there. No need for warnOnlyOnce still you might want to push warnings.

edit seems like we do not support warnings - no need to add them for that.

@@ -6,11 +6,13 @@
* found in the LICENSE file at https://angular.io/license
*/

import {ParseError, ParseSourceSpan} from '../parse_util';
import {ParseError, ParseErrorLevel, ParseSourceSpan} from '../parse_util';

This comment has been minimized.

Copy link
@vicb

vicb Sep 1, 2017

Contributor

revert

@@ -71,7 +71,8 @@ const TEMPLATE_ATTR_DEPRECATION_WARNING =

let warningCounts: {[warning: string]: number} = {};

function warnOnlyOnce(warnings: string[]): (warning: ParseError) => boolean {
/** @internal */
export function warnOnlyOnce(warnings: string[]): (warning: ParseError) => boolean {

This comment has been minimized.

Copy link
@vicb

vicb Sep 1, 2017

Contributor

revert

@vicb
Copy link
Contributor

left a comment

  • see inline comments
  • add DEPRACTIONS in the commit body with BEFORE/ACTION code snippet.
    Thanks

@ocombe ocombe force-pushed the ocombe:deprecate-i18n-comments branch from 1242c28 to 9131eaf Sep 1, 2017

@ocombe ocombe requested a review from juleskremer Sep 1, 2017

@mary-poppins

This comment has been minimized.

Copy link

commented Sep 1, 2017

@ocombe ocombe force-pushed the ocombe:deprecate-i18n-comments branch from 9131eaf to a7e4cec Sep 1, 2017

@juleskremer
Copy link
Contributor

left a comment

lgtm

@mary-poppins

This comment has been minimized.

Copy link

commented Sep 1, 2017

const details = comment.sourceSpan.details ? `, ${comment.sourceSpan.details}` : '';
// TODO(ocombe): use a log service once there is a public one available
console.warn(
` I18n comments are deprecated, use an <ng - container> element instead (${comment.sourceSpan.start}${details})`);

This comment has been minimized.

Copy link
@vicb

vicb Sep 5, 2017

Contributor

Any reason for the WS in <ng - container> ?

This comment has been minimized.

Copy link
@ocombe

ocombe Sep 5, 2017

Author Contributor

hu no sorry, mistake

@@ -405,6 +405,11 @@ export function main() {
});

describe('blocks', () => {
it('should console.warn if we use i18n comments', () => {

This comment has been minimized.

Copy link
@vicb

vicb Sep 5, 2017

Contributor

remove this test (no assertion, useless)

@vicb
Copy link
Contributor

left a comment

see inline comments

@ocombe ocombe force-pushed the ocombe:deprecate-i18n-comments branch 2 times, most recently from c8b3dec to 1bb6e44 Sep 5, 2017

@vicb

vicb approved these changes Sep 5, 2017

@vicb

This comment has been minimized.

Copy link
Contributor

commented Sep 5, 2017

@ocombe please check what is wrong with Travis

@ocombe ocombe force-pushed the ocombe:deprecate-i18n-comments branch from 1bb6e44 to bb3762b Sep 5, 2017

@mary-poppins

This comment has been minimized.

Copy link

commented Sep 5, 2017

@angular angular deleted a comment from mary-poppins Sep 5, 2017

@angular angular deleted a comment from mary-poppins Sep 5, 2017

@mhevery mhevery closed this in 66a5dab Sep 6, 2017

gkalpak added a commit to gkalpak/angular that referenced this pull request Sep 6, 2017

test(aio): fix docs example e2e test
Remove an assertion that is no longer true. This was supposed to be removed in angular#18998,
but was somehow dropped from 66a5dab (probably due to a bad rebase).

(Travis has been failing due to this since
https://travis-ci.org/angular/angular/jobs/272321759.)

gkalpak added a commit to gkalpak/angular that referenced this pull request Sep 6, 2017

test(aio): fix docs example e2e test
Remove an assertion that is no longer true. This was supposed to be removed in angular#18998,
but was somehow dropped from 66a5dab (probably due to a bad rebase).

(Travis has been failing due to this since
https://travis-ci.org/angular/angular/jobs/272321759.)

mhevery added a commit that referenced this pull request Sep 6, 2017

test(aio): fix docs example e2e test (#19070)
Remove an assertion that is no longer true. This was supposed to be removed in #18998,
but was somehow dropped from 66a5dab (probably due to a bad rebase).

(Travis has been failing due to this since
https://travis-ci.org/angular/angular/jobs/272321759.)

@ocombe ocombe deleted the ocombe:deprecate-i18n-comments branch Sep 11, 2017

@ocombe ocombe restored the ocombe:deprecate-i18n-comments branch Sep 15, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.