Initialize buildDependencies after beforeLoaders Hook

[swissmanu]

What kind of change does this PR introduce?

• Situation
  • The buildDependencies LazySet in NormalModule gets only initialized, if there is at least one Loader present for the module (https://github.com/webpack/webpack/blob/main/lib/NormalModule.js#L771-L773).
  • The beforeLoaders hook is called after this conditional intialization (https://github.com/webpack/webpack/blob/main/lib/NormalModule.js#L776)
  • Finally, all module loaders get added to buildDependencies (https://github.com/webpack/webpack/blob/main/lib/NormalModule.js#L823-L825), one after another.
• Problem
  • If there are no loaders configured upfront, but the beforeLoaders hook adds a loader (e.g., in a plugin), the Lines 823-825 throw an error, since this.buildDependencies was never initialized.
• Change
  • This PR ensures that the conditional initialization of buildDependencies happens after the beforeLoaders hook was executed.

Did you add tests for your changes?

🙋‍♂️ I read the test/README.md file and had a look at the existing test cases. Still, I require advice where I should put a test for my change. (I tend to test/cases/loaders, but I am really not sure if this would be the right place. :) )

Test case added to test/configCases/loaders/pr-14384

Does this PR introduce a breaking change?

No.

What needs to be documented once your changes are merged?

Nothing.

[linux-foundation-easycla]

The committers are authorized under a signed CLA.

• ✅ Manuel Alabor (262c245)

[webpack-bot]

For maintainers only:

• This needs to be documented (issue in webpack/webpack.js.org will be filed when merged)
• This needs to be backported to webpack 4 (issue will be created when merged)

[sokra]

🙋‍♂️ I read the test/README.md file and had a look at the existing test cases. Still, I require advice where I should put a test for my change. (I tend to test/cases/loaders, but I am really not sure if this would be the right place. :) )

test/configCases/loaders/ is a good place. e. g. start by cloning the module-default one.

[swissmanu]

test/configCases/loaders/ is a good place. e. g. start by cloning the module-default one.

thank you @sokra! i will have a look there.

[swissmanu]

finally added the test case. let me know if you need any more changes from my side in order to get this merged :-)

[alexander-akait]

@swissmanu CI failed, can you fix it?

[swissmanu]

@alexander-akait thanks for the nudge. i fixed one part of the problem... but don't understand how to fix the other. the config test cases template is giving me the following error:

  ● ConfigTestCases › loaders › pr-14384 › pr-14384 should compile

    No tests exported by test case

      566 |                                                                             }
      567 |                                                                             if (getNumberOfTests() < filesCount) {
    > 568 |                                                                                     return done(new Error("No tests exported by test case"));
          |                                                                                                    ^
      569 |                                                                             }
      570 |                                                                             done();
      571 |                                                                     })

     at test/ConfigTestCases.template.js:568:24

is there something in particular i am missing with my test case? i compared it with some others, but could not identify any significant differences imo :-/

[swissmanu]

i was not aware that the actual test file containing the assertions is also required via webpack. my dummy plugin injected the loader for all files loaded; hence, also for the test file and with that, the actual assertions were overwritten by the loader 🙈

anyway: @alexander-akait @sokra should be all green now. ✅

[alexander-akait]

@swissmanu can you fix tests on windows?

[swissmanu]

@alexander-akait lesson learned: wait for the CI results before informing you guys 😇 sry for the buzz.

everything looks good now, except that node-16 on linux could not install yarn: https://dev.azure.com/webpack/webpack/_build/results?buildId=14951&view=logs&jobId=fcdf401a-6636-533b-f723-8a3435a281c2

could anyone of you restart that particular job?

[webpack-bot]

@swissmanu The most important CI builds failed. This way your PR can't be merged.

Please take a look at the CI results from azure (1 errors / 6 warnings) and appveyor (success) and fix these issues.

[alexander-akait]

All is fine, ignore random fails (when it happens for only one platform) 👍

[sokra]

Thanks