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

Irritating warning when setting missingTranslation in AotPlugin options #9159

Conversation

mhoyer
Copy link

@mhoyer mhoyer commented Jan 10, 2018

When using missingTranslation in options for AotPlugin constructor the plugin will print a confusing warning when angular (and CLI) is already at version 5 or higher:

The --missing-translation parameter will be ignored because is only with Angular version 4.2.0 or higher.
If you want to use it, please your Angular version.

Example in webpack.config.js:

    ...
    const aotOptions = {
        tsConfigPath: './tsconfig.json',
        entryModule: ...
        exclude: [...],
        locale: 'en',
        i18nFile: 'messages.en.xlf',
        i18nFormat: 'xlf',
        missingTranslation: 'warning', // <<< this will produce the warning
    }
    new AotPlugin(aotOptions);

@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

…gTranslation option validation

When using `missingTranslation` in options for AotPlugin constructor the plugin will print a warning when angular (and cli) is already at version 5 or higher.
@mhoyer mhoyer force-pushed the warning-for-missingTranslation-due-to-wrong-version-check branch from 3dcbc73 to edae424 Compare January 10, 2018 22:21
@googlebot
Copy link

CLAs look good, thanks!

@clydin
Copy link
Member

clydin commented Jan 11, 2018

It shouldn’t be showing the warning. However, AngularCompilerPlugin should be used for Angular 5+. AotPlugin is intended for Angular 2 & 4.

@mhoyer
Copy link
Author

mhoyer commented Jan 11, 2018

@clydin you are right. I missed that line in the README.md:

Angular version 5 and up, use AngularCompilerPlugin:

Hence, the whole AotPlugin maybe should fail to initialize in general - in case the underlying version of @angular/compiler-cli already is 5.0 or higher? Or does is there any use case where it makes sense for someone to keep using AotPlugin together with ng5+?

And was it on purpose to keep both versions (AotPlugin and new AngularCompilerPlugin) in one version branch to be released and published side-by-side? E. g. defining clear peerDependencies to a required @angular/compiler-cli would make more sense IMHO.

In general it feels strange to have in-code verification for particular version number of peer installed dependencies when there is actually infrastructure for dependency/version management through NPMs package.json available.

@clydin
Copy link
Member

clydin commented Jan 11, 2018

The package is intended to provide support for Angular 2 through 5 (and eventually beyond). The CLI also supports building Angular 2, 4, and 5 based projects. So both are required.

@filipesilva
Copy link
Contributor

filipesilva commented Jan 25, 2018

@mhoyer no, there isn't any usecase for using AotPlugin with Angular v5+.

At the moment we can't set peer dependencies on @angular/compiler-cli because we use the same global and local CLI package, and that complicates things (see #7625 (comment)). That's why we have in code verification of versions.
We're moving to separate this though.

I agree we could have some error being thrown in AoTPlugin though. Would you be interested in changing your PR to do that?

Copy link
Contributor

@filipesilva filipesilva left a comment

Choose a reason for hiding this comment

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

See comment above.

@filipesilva filipesilva self-assigned this Jan 25, 2018
@filipesilva filipesilva self-assigned this Feb 22, 2018
@clydin
Copy link
Member

clydin commented Apr 4, 2018

Closing as the package has been moved to the devkit project and Angular 5 is now the minimum version.

@clydin clydin closed this Apr 4, 2018
@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 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants