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-cli): ngcc - make logging more configurable #29591

Closed
wants to merge 1 commit into from

Conversation

@petebacondarwin
Copy link
Member

commented Mar 29, 2019

This allows CLI usage and integrations like webpack plugins
to filter excessive log messages.

// FW-1198

@ngbot

This comment has been minimized.

Copy link

commented Mar 29, 2019

I see that you just added the PR action: merge label, but the following checks are still failing:
    failure conflicts with base branch "master"
    pending missing required labels: cla: yes
    pending missing required status "ci/circleci: build"
    pending missing required status "ci/circleci: lint"
    pending missing required status "ci/circleci: publish_snapshot"
    pending missing required status "ci/angular: size"
    pending missing required status "cla/google"
    pending missing required status "google3"

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

@petebacondarwin petebacondarwin force-pushed the petebacondarwin:FW-1198 branch 2 times, most recently from d520e0d to 3259fc6 Mar 29, 2019

@petebacondarwin

This comment has been minimized.

Copy link
Member Author

commented Mar 29, 2019

I'm going to open up the API a bit more...

feat(compiler-cli): ngcc - make logging more configurable
This allows CLI usage to filter excessive log messages
and integrations like webpack plugins to provide their own logger.

// FW-1198

@petebacondarwin petebacondarwin force-pushed the petebacondarwin:FW-1198 branch from 3259fc6 to 92d26d3 Mar 29, 2019

@alan-agius4
Copy link
Contributor

left a comment

LGTM.

export const WARN = `${YELLOW}Warning:${RESET}`;
export const ERROR = `${RED}Error:${RESET}`;

export enum LogLevel {

This comment has been minimized.

Copy link
@gkalpak

gkalpak Mar 30, 2019

Member

Do we need a log LogLevel?

This comment has been minimized.

Copy link
@petebacondarwin

petebacondarwin Mar 30, 2019

Author Member

I don't know. We don't currently appear to :-) We can always add it later.

@alxhub

alxhub approved these changes Apr 1, 2019

* The function will recurse into directories that start with `@...`, e.g. `@angular/...`.
* @param sourceDirectory An absolute path to the root directory where searching begins.
*/
private walkDirectoryForEntryPoints(sourceDirectory: AbsoluteFsPath): EntryPoint[] {

This comment has been minimized.

Copy link
@alxhub

alxhub Apr 1, 2019

Contributor

I don't see these functions using the logger at all - is the fact that they're now private class methods instead of top-level functions related to the logging change?

This comment has been minimized.

Copy link
@petebacondarwin

petebacondarwin Apr 1, 2019

Author Member

Yes the getEntryPointsForPackage() method calls getEntryPointInfo(), which needs a logger.

I was really unhappy with having to pass the Logger down to that free standing function but as much as a played around I could not get a better architecture, without a significant refactoring of this whole bit of the codebase.

@jasonaden jasonaden closed this in 8d3d75e Apr 1, 2019

@petebacondarwin petebacondarwin deleted the petebacondarwin:FW-1198 branch Apr 1, 2019

wKoza added a commit to wKoza/angular that referenced this pull request Apr 17, 2019

feat(compiler-cli): ngcc - make logging more configurable (angular#29591
)

This allows CLI usage to filter excessive log messages
and integrations like webpack plugins to provide their own logger.

// FW-1198

PR Close angular#29591
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.