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

fix: getLogger not defined in child loader #130

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

benjamin-hodgson
Copy link

@benjamin-hodgson benjamin-hodgson commented Dec 8, 2021

This PR fixes #10 and #185.

This PR contains a:

  • bugfix
  • new feature
  • code refactor
  • test update
  • typo fix
  • metadata update

Motivation / Use-Case

Currently thread-loader is not compatible with less-loader, as outlined in #10 which was closed without resolution.

less-loader calls this.getLogger, which worker.js hadn't defined when calling loaderRunner.runLoaders, so it crashed. I set up getLogger to simply write to the child process's console, instead of messaging back to the parent process - the PoolWorker doesn't currently have access to the parent loader's getLogger function and it seemed like too big of a change to pass it in. I'm open to changing this (but would prefer to get this fix released as soon as possible since it's a live issue.)

Thanks!

Breaking Changes

None

Additional Info

@benjamin-hodgson
Copy link
Author

Hello, just following up on this bugfix. This is a live issue affecting any build configurations using thread-loader with less-loader. Thanks!

console.debug(`${name}: ${message}`);
},
};
},
Copy link
Member

Choose a reason for hiding this comment

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

Ideally we should run webpack logger here, otherwise options from configuration for logging will not work

Copy link
Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Can we map methods created logger, we can have more them error/warn/log/debug

Copy link

Choose a reason for hiding this comment

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

Just bumping this PR for some clarification.

Can we map methods created logger, we can have more them error/warn/log/debug

@alexander-akait Do you mean to just loop through the list of methods instead of manually specifying them? e.g.

const methods = ['error', 'warn', 'log', 'debug']];
const logger = {}

for (const key of methods ) {
    logger[key] = ...
}

return logger;

Copy link

Choose a reason for hiding this comment

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

@alexander-akait Nudging this PR to see what's blocking it?

@abalbanyan
Copy link

@alexander-akait Would really appreciate a resolution to this. It's blocking my team from upgrading less-loader.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Issues with Less-Loader
4 participants