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.

Sign up for GitHub

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

Jump to bottom

Update plugins to use Webpack Logger #202

Closed
wants to merge 2 commits into from
Closed

Update plugins to use Webpack Logger #202

wants to merge 2 commits into from

Conversation

GuilleAngulo
Copy link
Collaborator

@GuilleAngulo GuilleAngulo commented Jun 17, 2021 •
edited

Description

It updates plugins logging to use Webpack API. There are several methods available: it is possible to use info(), warn() or error() for important messages and log() for messages only displayed when debug mode is enabled.

Problem with plugins

While testing I realized there is a problem with the configuration: they all share the same config object. For example for the following config:

[indexSearch, { outputDirectory: './public/search'], [feed], [sitemap], [socialImages]

All plugins will create files at ./public/search, because seems like the config object is passed sequentially from one plugin to the next. In order to fix this problem it is added a reset of the properties just after destructuring, to assure that the property will be undefined if nothing is passed. If a value is passed to other plugin it will override any previous value, so I think now is working as expected. Probably not the best approach/solution, but a quick fix for now, would love to hear your thoughs.

if (Object.prototype.hasOwnProperty.call(nextConfig, 'outputDirectory')) nextConfig.outputDirectory = undefined; 
if (Object.prototype.hasOwnProperty.call(nextConfig, 'outputName')) nextConfig.outputName = undefined; 
if (Object.prototype.hasOwnProperty.call(nextConfig, 'debug')) nextConfig.debug = undefined;

Debug config

If a debug boolean variable is passed to the plugin config, additional logs will be displayed. Reference: https://webpack.js.org/configuration/other-options/#debug

Named plugins

For each plugin it is created a new class extending WebpackPluginCompiler class:

class FeedPlugin extends WebpackPluginCompiler

Just updated this part because I noticed that all plugins were pushed into webpack with same name, and maybe this could have some side effects at some point? Not sure of that, I can revert this change if you think its not necessary. (Reference: https://webpack.js.org/contribute/writing-a-plugin/#creating-a-plugin)

Before After
plugins plugins-after

Would love if you could help me testing it and any thoughts you might have about it =)

Fixes #189

@GuilleAngulo GuilleAngulo added on hold bug Something isn't working invalid This doesn't seem right and removed on hold bug Something isn't working labels Jun 17, 2021
@GuilleAngulo GuilleAngulo removed the invalid This doesn't seem right label Jun 18, 2021
@colbyfayock
Copy link
Owner

hey @GuilleAngulo is this good to go? saw some straggler commits, but noticed you removed the invalid tag

@GuilleAngulo
Copy link
Collaborator Author

Yeah, sorry about that @colbyfayock . What happened is that when I opened the PR the CI failed due to an unrecognized optional chaining. So while resolving it I wanted to mark it with some label to notify that it was in progress. I didnt know all label changes appear in the history...And I was testing which one I should use 🤓 .

I think everything should work as expected but I'd love if you could test it also =)

About commits, should I squash both into one to make the story clearer?

@colbyfayock
Copy link
Owner

hey this is looking great so far

one thing i noticed is when running build locally its like some of the logs aren't getting a new line:
image

are you seeing that too? wonder if theres any way to clean that up?

and some have this <i>
image

the above was with debug mode, here's without

image

good catch with the config. i think we might want to think how to more cleanly do the config part, but maybe we can do that in a separate PR?

if we have to set it as undefined like that it makes me think we're doing it wrong and probably best to fix it to be the "right way"

dont worry about squashing

@GuilleAngulo
Copy link
Collaborator Author

hey sorry for the delay,

About the visuals:
I didn't find a way to make each log in one line when doing the build... and about the tags seems like this is the default visual for info, warn and error.

shot_210629_104955

So these things added to the fact that customizing debug is a bit tricky(since it can log other parts of next/webpack) make me think if it would be a good idea to close this issue for now. What do you think? Maybe I can try to improve the log that is already in the project (separating methods, using chalk, ...)

About the config thing totally agree with you, doesnt seem like a correct way to solve it. I'll open a separate issue and try to figure out how can we fix it.

@colbyfayock
Copy link
Owner

thats strange, i would have thought it would be much more streamlined. yeah im cool with holding on it and tidying up manually if thats what you prefer. im cool with what you decide there

@GuilleAngulo
Copy link
Collaborator Author

I'm closing this for now, just because it seems that having our custom logger allows more freedom on how to output logs. Anyway I have all the information in case we need to get back to it. I think it is preferable to focus on the problem with the variables in plugins.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Update plugins to use Webpack Logger interface
2 participants
@GuilleAngulo @colbyfayock