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

Several ngcc fixes #34014

Closed
wants to merge 3 commits into from
Closed

Several ngcc fixes #34014

wants to merge 3 commits into from

Conversation

@JoostK
Copy link
Member

JoostK commented Nov 23, 2019

See individiual commits for descriptions.

@ngbot ngbot bot added this to the needsTriage milestone Nov 23, 2019
@googlebot googlebot added the cla: yes label Nov 23, 2019
@JoostK JoostK marked this pull request as ready for review Nov 23, 2019
@JoostK JoostK requested review from angular/fw-compiler as code owners Nov 23, 2019
Copy link
Member

petebacondarwin left a comment

Great stuff @JoostK

@JoostK JoostK force-pushed the JoostK:ngcc-migration-errors branch from 07f22c9 to 63a9eb3 Nov 24, 2019
Copy link
Member

gkalpak left a comment

Some minor comments - otherwise lgtm 👌

@@ -248,11 +249,13 @@ export function mainNgcc(
const result = transformer.transform(bundle);
if (result.success) {
if (result.diagnostics.length > 0) {
logger.warn(ts.formatDiagnostics(result.diagnostics, bundle.src.host));
logger.warn(replaceTsWithNgInErrors(
ts.formatDiagnostics(result.diagnostics, bundle.src.host), /* colorized */ false));

This comment has been minimized.

Copy link
@gkalpak

gkalpak Nov 25, 2019

Member

OOC, why using colorized: false here?

This comment has been minimized.

Copy link
@JoostK

JoostK Nov 26, 2019

Author Member

Because diagnostics are formatted using ts.formatDiagnostics, not ts.formatDiagnosticsWithColorsAndContext. This difference has been mentioned in the params documentation for colorized.

if (colorized) {
return errors.replace(ERROR_CODE_COLORIZED_MATCHER, '$1NG$2');
} else {
return errors.replace(ERROR_CODE_MATCHER, '$1NG$2');

This comment has been minimized.

Copy link
@gkalpak

gkalpak Nov 25, 2019

Member

Why not omit the colorized param and use a RegExp that can match both versions?
(Having to pass in colorized is error prone and can easily get out-of-sync with the actual implementation.)

This comment has been minimized.

Copy link
@JoostK

JoostK Nov 26, 2019

Author Member

If all is well, the non-colorized regex would work just fine for colorized diagnostics, it's just that the colorized regex is less likely to incorrectly replace some random string that happens to contain " TS-99##: ". So we could drop the colorized version. I kept it the way it was as I didn't want to change the logic of how it already worked in ngtsc, as I presume it was intentionally written the way it was.

For non-colorized messages that are used in ngcc though, the colorized regex wouldn't work. Hence I added an additional one. I choose to do this in the same function, as that achieves a centralized place where this replacement occurs. One could argue that it should be two separate functions, the non-colorized one potentially only present in ngcc, however I prefered the centralized logic.

I am not convinced we should change any of this, if you feel differently please let me know :-)

This comment has been minimized.

Copy link
@gkalpak

gkalpak Nov 27, 2019

Member

Is there any particular reason we use non-colorized formatting in ngcc?

This comment has been minimized.

Copy link
@JoostK

JoostK Dec 8, 2019

Author Member

@gkalpak As mentioned on Slack, there really isn't. I have changed ngcc to report colorized diagnostics with colors, you can see them in action here.

When ngcc is analyzing synthetically inserted decorators from a
migration, it is typically not expected that any diagnostics are
produced. In the situation where a diagnostic is produced, however, the
diagnostic would not be reported at all. This commit ensures that
diagnostics in migrations are reported.
@JoostK JoostK force-pushed the JoostK:ngcc-migration-errors branch from 63a9eb3 to a98e65f Dec 7, 2019
JoostK added a commit to JoostK/ngcc-validation that referenced this pull request Dec 8, 2019
JoostK added 2 commits Nov 23, 2019
Replaces the "TS-99" sequence with just "NG", so that error codes are
logged correctly.
The undecorated child migration creates a synthetic decorator, which
contained `"exportAs": ["exportName"]` as obtained from the metadata of
the parent class. This is a problem, as `exportAs` needs to specified
as a comma-separated string instead of an array. This commit fixes the
bug by transforming the array of export names back to a comma-separated
string.
@JoostK JoostK force-pushed the JoostK:ngcc-migration-errors branch from a98e65f to fa66e4b Dec 8, 2019
@petebacondarwin petebacondarwin requested review from gkalpak and alxhub Dec 9, 2019
@gkalpak
gkalpak approved these changes Dec 9, 2019
@JoostK

This comment has been minimized.

Copy link
Member Author

JoostK commented Dec 9, 2019

merge-assistance: Current reviews should be sufficient, as I'm code owner of fw-compiler (and it's only a comment change).

@AndrewKushnir

This comment has been minimized.

Copy link
Contributor

AndrewKushnir commented Dec 9, 2019

AndrewKushnir added a commit that referenced this pull request Dec 10, 2019
Replaces the "TS-99" sequence with just "NG", so that error codes are
logged correctly.

PR Close #34014
AndrewKushnir added a commit that referenced this pull request Dec 10, 2019
…#34014)

The undecorated child migration creates a synthetic decorator, which
contained `"exportAs": ["exportName"]` as obtained from the metadata of
the parent class. This is a problem, as `exportAs` needs to specified
as a comma-separated string instead of an array. This commit fixes the
bug by transforming the array of export names back to a comma-separated
string.

PR Close #34014
AndrewKushnir added a commit that referenced this pull request Dec 10, 2019
When ngcc is analyzing synthetically inserted decorators from a
migration, it is typically not expected that any diagnostics are
produced. In the situation where a diagnostic is produced, however, the
diagnostic would not be reported at all. This commit ensures that
diagnostics in migrations are reported.

PR Close #34014
AndrewKushnir added a commit that referenced this pull request Dec 10, 2019
Replaces the "TS-99" sequence with just "NG", so that error codes are
logged correctly.

PR Close #34014
AndrewKushnir added a commit that referenced this pull request Dec 10, 2019
…#34014)

The undecorated child migration creates a synthetic decorator, which
contained `"exportAs": ["exportName"]` as obtained from the metadata of
the parent class. This is a problem, as `exportAs` needs to specified
as a comma-separated string instead of an array. This commit fixes the
bug by transforming the array of export names back to a comma-separated
string.

PR Close #34014
josephperrott added a commit to josephperrott/angular that referenced this pull request Dec 11, 2019
When ngcc is analyzing synthetically inserted decorators from a
migration, it is typically not expected that any diagnostics are
produced. In the situation where a diagnostic is produced, however, the
diagnostic would not be reported at all. This commit ensures that
diagnostics in migrations are reported.

PR Close angular#34014
josephperrott added a commit to josephperrott/angular that referenced this pull request Dec 11, 2019
Replaces the "TS-99" sequence with just "NG", so that error codes are
logged correctly.

PR Close angular#34014
josephperrott added a commit to josephperrott/angular that referenced this pull request Dec 11, 2019
…angular#34014)

The undecorated child migration creates a synthetic decorator, which
contained `"exportAs": ["exportName"]` as obtained from the metadata of
the parent class. This is a problem, as `exportAs` needs to specified
as a comma-separated string instead of an array. This commit fixes the
bug by transforming the array of export names back to a comma-separated
string.

PR Close angular#34014
@angular-automatic-lock-bot

This comment has been minimized.

Copy link

angular-automatic-lock-bot bot commented Jan 10, 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 Jan 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.