Add generated assets to main compiler #59

lalexdotcom · 2022-09-02T10:34:25Z

The assets generated by childCompilation (basically the JS files) are now added to the main compilation assets, allowing other Webpack plugins to be aware of it.

FIXES: #58

simoneb

link the issue to the PR
write a test :)

ShogunPanda · 2022-09-02T10:49:26Z

Other than what Simone said, rest LGTM! Nice job!

lalexdotcom · 2022-09-02T10:50:15Z

OK.

How can I link the PR to the issue after creation?
Should I create new test or editing the main test by checking the assets is enough?

ShogunPanda · 2022-09-02T10:51:28Z

Just write a Fixes: #NUMBER in the PR description so that GitHub will link it.
Up to you, whatever you find most readable and comfortable.

lalexdotcom · 2022-09-02T11:09:22Z

Actually, the used hook is wrong, sometimes never get called...

mcollina · 2022-09-02T12:13:32Z

@ShogunPanda what test can we add for this feature?

ShogunPanda · 2022-09-02T12:14:40Z

Well, since the original problem was that the pino assets disappeared, maybe we can just check they don't when used in conjunction of the web pack cleaning plugin.

lalexdotcom · 2022-09-02T13:56:37Z

The test would be to check if the assets property of returned stats contains pino generated files....

I had to leave the computer, but I'll work on that by the end of the day... (my day, in Europe 😉)

lalexdotcom · 2022-09-02T13:57:38Z

Clean plugin was just the plug-in who led to find this issue, but other plugins may need to have accurate assets from compilation....

lalexdotcom · 2022-09-02T17:38:09Z

I'm actually stuck. When I add an asset to the main compilation assets, two files are generated with different hashes (one before adding the footer code, and one after)...

I have to spend more time on it, it was a bit too soon to send the PR. 😅

Child compilation assets are generated in memory Assets are now addded to the main compilation allowing other plugins to be aware of those additional files ...but cache test doesn't pass anymore, don't know where to handle it

lalexdotcom · 2023-01-18T09:22:20Z

I finally had time to spend on this...
It's nearly a complete refactoring to be sure to use the correct hooks, but the webpack documentation is so poor about child compilers, hooks and cache... 😕

@gdorsi : as you wrote the PR to handle cache in this plugin, could you have a look at my code? I really don't understand how to handle the cache... and if you could explain to me how it works, I would really appreciate! 😇

simoneb · 2023-01-18T11:13:04Z

@lalexdotcom meanwhile it might be worth looking into why CI fails

lalexdotcom · 2023-01-18T11:38:38Z

You're right, it's linting issues: the variables used by cache handling were still there, and sometimes I prefer having no brackets for a simple if
I must admit I didn't launch the formatting & linting tasks. 😅
It's done in my new commit...

mcollina · 2023-01-18T11:39:03Z

src/index.js

@@ -2,7 +2,7 @@ const webpack = require('webpack')
 const CommonJsRequireDependency = require('webpack/lib/dependencies/CommonJsRequireDependency')
 const { createFsFromVolume, Volume } = require('memfs')
 const { sep, dirname, relative } = require('path')
-const { cache } = require('webpack')
+// const { cache } = require('webpack') // Cache handling missing


why is this commented out?

As I said in my commit, I couldn't find where to handle the cache in the refactoring: #59 (comment)
To be able to emit assets from the child compilation, I had to use different webpack hooks...

Actually, it's nearly a complete refactoring, I thought it would be more readable this way:

I got into the (too light) Webpack hooks documentation, and used more appropriate hooks to handle additional assets

I thought that having functions declared further in code but used only once doesn't help readability. I used the function body directly in the webpack hooks

Please let me know if you think I should have kept the previous code "style"...

gdorsi

Happy to help!

Well done @lalexdotcom, switching to webpack.EntryPlugin is a great improvement!

WIth webpack.EntryPlugin the new files are no more considered external so the cache is already handled by webpack (TBH I don't understand 100% the referenced code, but since the tests are passing I think there is no need to go deeper)

As general advice for your future contributions, try to keep the style changes in a separate PR because they make the review process way harder.

gdorsi · 2023-01-18T11:58:57Z

src/index.js

+        childCompiler.inputFileSystem = compiler.inputFileSystem
+        // Generate files in memory
+        // It will be written to output path by main compilation
+        childCompiler.outputFileSystem = fs


mmmhh, I'm not sure that overriding the default fs is a good thing.

Why did you made this change?

Hey @gdorsi

I thought that avoiding writing intermediate assets to the file system and keeping them in memory would be a good idea...

Actually, not all the running tests are passing: the cached build test doesn't pass (this is why I tagged you in my comment). The second build is generating new files...

Noted for changing style in PRs. Actually, it helped me to understand the process of the plugin, so for me it was kinda part of the feature...

Could you take a look on the cache handling part? And if you could explain to me how it works...

And the EntryPlugin was already there as I remember (addExternalFile and trackInclusions functions)

Actually, not all the running tests are passing: the cached build test doesn't pass (this is why I tagged you in my comment). The second build is generating new files...

Weird, I tried to run the tests locally and they are passing:

test/cached-build.test.js it should correctly generate all required pino files when the cache is enabled on Webpack ✓ should not error ✓ should not error ✓ expect falsey value ✓ expect truthy value ✓ expect truthy value ✓ expect truthy value ✓ expect truthy value ✓ should match pattern provided ✓ should match pattern provided

could you share the errors you are getting?

Still not for me:

test/cached-build.test.js it should correctly generate all required pino files when the cache is enabled on Webpack ✓ should not error ✓ should not error ✓ expect falsey value ✓ expect truthy value 1) expect truthy value ✓ expect truthy value ✓ expect truthy value ✓ should match pattern provided ✓ should match pattern provided

Maybe there's a cache folder I did'nt clear, but I don't see any...

Ah ok, I missed this so now I understand why the memory fs is useful (would be worth a comment in the code) and why the tests fail if it is disabled.

Good call!

Have you find out why the tests are not passing on your local?

If not I could try to help.

Could you share your node version and the lockfile?

The tests are now passing on my local computer BUT SHOULDN'T: two 'first-xxxx.js' and 'abc/cde/second-xxxxx.js' files are generated (when using cache)...
As readdirSync is returning filenames in alphabetical order, depending on the hash, tests could succeed when it shouldn't... for example they should fail if there's several file starting with 'first-'...

A way to help me would be to test with no temp dir, look at the output and see why the file generated on the second build with cache enabled has a different name: as I said, I don't really understand how the webpack cache works and there's poor documentation on that... does the childCompiler use cache?

Ah ok, I missed this so now I understand why the memory fs is useful (would be worth a comment in the code) and why the tests fail if it is disabled.

You're perfectly right, I added a comment on that in a new commit, let me know if it's clear enough

look at the output and see why the file generated on the second build with cache enabled has a different name

If the name is generated using the content hash it is normal for them to have different names.

I don't really understand how the webpack cache works and there's poor documentation on that... does the childCompiler use cache?

The poor docs is what make writing Webpack plugins so hard.
I did this one year ago and don't remember these parts, only that you need to check the Webpack source code to understand what's happening.

look at the output and see why the file generated on the second build with cache enabled has a different name

If the name is generated using the content hash it is normal for them to have different names.

What I don't understand is that file content hasn't changed, so the hash shouldn't have either...
I'm gonna try to dive deeper in the cache handling, but don't know when I'll be able to fix this...

A more consistent variable name

gdorsi

There are too many unrelated changes here making this PR is too hard to be reviewed properly.

Let's focus on what we need to fix the issue and keep the rest for future contributions.

ShogunPanda · 2023-01-18T14:44:07Z

The code refactoring looks good from what I can see. The refactoring was huge and thus I trust the CI.
I think we can resolve conflicts in #58 in a separate PR. Once tests passes everywhere, let's land this and move on.

@lalexdotcom Thanks for the PR, this was huge.

ShogunPanda · 2023-01-18T14:44:57Z

@gdorsi @lalexdotcom Since CI is already passing, I wait for the green light from you to land this.

simoneb · 2023-01-18T14:45:24Z

given the extent of the changes, as soon as we're happy with the PR it may be more cautious to release this as a prerelease. @lalexdotcom is this supposed to contain semver breaking changes?

lalexdotcom · 2023-01-18T14:54:54Z

The code refactoring looks good from what I can see. The refactoring was huge and thus I trust the CI. I think we can resolve conflicts in #58 in a separate PR. Once tests passes everywhere, let's land this and move on.

@lalexdotcom Thanks for the PR, this was huge.

Hi @ShogunPanda, thanks for your feedback.
@gdorsi pointed out that the CI seem to pass because of an issue with memoryFs #59 (comment)

This PR already solves #58, but in my local config AND in a local devcontainer (Node 18-bullseye for VSCode), cache tests don't pass... this is a regression and maybe it would not be wasted time for reliability to spend more time on this, maybe on different user machine, using isolated environment?

For semver, can adding a new dependency be considered as a breaking change? If not, no breaking change...

simoneb · 2023-01-18T14:57:13Z

For semver, can adding a new dependency be considered as a breaking change? If not, no breaking change...

thanks for the clarification on this point.

gdorsi · 2023-01-19T10:51:12Z

@gdorsi @lalexdotcom Since CI is already passing, I wait for the green light from you to land this.

@ShogunPanda there is still the issue of a test failing on @lalexdotcom local.

Once we have sorted that out the PR is good to go for me.

This hook is called even if cache is enabled

lalexdotcom · 2023-01-20T08:13:05Z

I think I nailed it! 😎

When using cache, the buildModule hook isn't called if the module is cached. As I was using this hook to populate the childCompiler entries, the second build was generating another __bundlerPathsOverrides object (empty, because no module was rebuilt)... different object ➡️ different file content ➡️ different name hash ➡️ different file 😓

I'm now using the finishModules hook to populate the childCompiler entries in one loop on all the modules, even the one that were previously cached... it allows also to not have to handle cache by hand...

lalexdotcom · 2023-01-20T08:17:27Z

As this PR was originally made to fix #58, i also updated the clean-plugin.test to perform 2 builds and be sure that cold start and restart have the same behavior, which is required for watch mode...

mcollina · 2023-01-20T08:26:16Z

@ShogunPanda PTAL

simoneb · 2023-01-24T15:24:27Z

@ShogunPanda @lalexdotcom are we happy with this PR then? I assume this needs to go in before #83 ?

lalexdotcom · 2023-01-24T15:41:13Z

@simoneb OK for me.

I would say we can merge #59 then I'll make another commit to #83 with updated clean-plugin.test.js...

mcollina · 2023-01-24T16:06:59Z

go for it!

* Tests refactoring * Renamed 'testPlan' var to 'planEstimation' * Take constants out of the createTests function * Use ?? operator instead of ??= nodejs@14 compatibility * Update new test introduced in #59

Add generated assets to main compiler

c67e291

lalexdotcom mentioned this pull request Sep 2, 2022

Conflict with CleanWebpackPlugin #58

Closed

fix :Well formated additionalAssets handler

ba008a3

simoneb reviewed Sep 2, 2022

View reviewed changes

Refactoring with regression

1502f28

Child compilation assets are generated in memory Assets are now addded to the main compilation allowing other plugins to be aware of those additional files ...but cache test doesn't pass anymore, don't know where to handle it

Merge branch 'pinojs:main' into add_assets

6fe5ee4

Resolve linting issues

8fa0f0d

mcollina reviewed Jan 18, 2023

View reviewed changes

gdorsi reviewed Jan 18, 2023

View reviewed changes

A few additional comments

c55050c

A more consistent variable name

gdorsi suggested changes Jan 18, 2023

View reviewed changes

Add comment to the memFs use

d7b5bd1

Create childCompiler entries in finishModules hook

57ac37e

This hook is called even if cache is enabled

lalexdotcom mentioned this pull request Jan 20, 2023

Tests #83

Merged

lalexdotcom added 2 commits January 20, 2023 09:24

Rename explicit import test

d6b4205

typo...

2bed472

mcollina requested a review from ShogunPanda January 20, 2023 08:26

ShogunPanda approved these changes Jan 24, 2023

View reviewed changes

mcollina merged commit 83736ab into pinojs:main Jan 24, 2023

lalexdotcom added a commit to lalexdotcom/pino-webpack-plugin that referenced this pull request Jan 24, 2023

Update new test introduced in pinojs#59

33d3ad4

lalexdotcom added a commit to lalexdotcom/pino-webpack-plugin that referenced this pull request Jan 24, 2023

Update new test introduced in pinojs#59

b39ddc4

github-actions bot mentioned this pull request Jan 26, 2023

Release pending! #73

Closed

Add generated assets to main compiler #59

Add generated assets to main compiler #59

Conversation

lalexdotcom commented Sep 2, 2022 • edited Loading

simoneb left a comment

Choose a reason for hiding this comment

ShogunPanda commented Sep 2, 2022

lalexdotcom commented Sep 2, 2022

ShogunPanda commented Sep 2, 2022

lalexdotcom commented Sep 2, 2022

mcollina commented Sep 2, 2022

ShogunPanda commented Sep 2, 2022

lalexdotcom commented Sep 2, 2022

lalexdotcom commented Sep 2, 2022

lalexdotcom commented Sep 2, 2022

lalexdotcom commented Jan 18, 2023 • edited Loading

simoneb commented Jan 18, 2023

lalexdotcom commented Jan 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gdorsi left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gdorsi Jan 19, 2023 • edited Loading

Choose a reason for hiding this comment

lalexdotcom Jan 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gdorsi left a comment • edited Loading

Choose a reason for hiding this comment

ShogunPanda commented Jan 18, 2023

ShogunPanda commented Jan 18, 2023

simoneb commented Jan 18, 2023

lalexdotcom commented Jan 18, 2023

simoneb commented Jan 18, 2023

gdorsi commented Jan 19, 2023

lalexdotcom commented Jan 20, 2023

lalexdotcom commented Jan 20, 2023

mcollina commented Jan 20, 2023

simoneb commented Jan 24, 2023

lalexdotcom commented Jan 24, 2023

mcollina commented Jan 24, 2023

lalexdotcom commented Sep 2, 2022 •

edited

Loading

lalexdotcom commented Jan 18, 2023 •

edited

Loading

lalexdotcom commented Jan 18, 2023 •

edited

Loading

gdorsi left a comment •

edited

Loading

gdorsi Jan 19, 2023 •

edited

Loading

lalexdotcom Jan 19, 2023 •

edited

Loading

gdorsi left a comment •

edited

Loading