Exposing output path to subsequent loaders

[cduivis]

This PR contains a:

• [ ] bugfix
• [x] new feature
• [x] code refactor
• [x] test update
• [ ] typo fix
• [ ] metadata update

Motivation / Use-Case

In certain projects you want to parse a lot of files which have their own dependencies. When using file-loader you can tell it into which folder the files need to be written. The problem is the loader processing the files, doesn't know to which paths all the files are written and thus cannot make a correct relative path dependency on those files. By pitching the calculated output path of file-loader to all subsequent loaders they then can use this information to correctly resolve a relative path.

In my case I have multiple .html files which include some .less files specific to it. These files need to be placed in the same folder. I don't want to worry inside the .html file about the setup of webpack so I just want to include the .less file relatively. For this to work correctly extract-loader should know where the .html file needs to be written to create correct relative paths for the .less files.

I've already setup a version of extract-loader which uses these changes to resolve paths relatively.

Breaking Changes

No breaking changes.

Additional Info

There can be parts of the path which can't be correctly resolved yet like the [hash] as it relies on the content of the file for the hash to be formed. The output path for this file will just keep [hash] as part of the output path name. As it's unique to this file it will still resolve relative paths correctly.

[jsf-clabot]

All committers have signed the CLA.

[codecov[bot]]

Codecov Report

Merging #321 into master will not change coverage. The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #321   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           2      2           
  Lines          25     44   +19     
  Branches        9     11    +2     
=====================================
+ Hits           25     44   +19

Impacted Files	Coverage Δ
src/cjs.js	100% <100%> (ø)	:arrow_up:
src/index.js	100% <100%> (ø)	:arrow_up:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update d699243...3562ca8. Read the comment docs.

[codecov[bot]]

Codecov Report

Merging #321 into master will not change coverage. The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #321   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           2      2           
  Lines          25     44   +19     
  Branches        9     11    +2     
=====================================
+ Hits           25     44   +19

Impacted Files	Coverage Δ
src/cjs.js	100% <100%> (ø)	:arrow_up:
src/index.js	100% <100%> (ø)	:arrow_up:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update d016daa...5718a4c. Read the comment docs.

[cduivis]

@evilebottnawi a review from anyone of the team would be appreciated.

[cduivis]

Because of the strict building rules and committing rules that wasn't really an option. Firstly, I need to adhere to the commit rules your repository has configured.

Secondly, your automated PR bot required me to fix the security vulnerabilities that NPM reported before it would accept the PR.

So what would you suggest?

[alexander-akait]

@cduivis avoid this, it is chore job, feel free if you CI was failed, before merge i fix it and rebase you branch

[cduivis]

So if I understand correctly you want me to undo the audit changes, and then you will re-fix them on your end?

[alexander-akait]

@cduivis yes, you are right

[cduivis]

Ok I've removed the audit changes as requested ;)

[alexander-akait]

Introduce pitch is breaking change, just create util directory with utils and export their as part of loader api

[cduivis]

Could you explain to me how or why this is a breaking change? The file loader still does exactly the same as it did before it only pitches data to subsequent loaders, which can be useful for them. This means if you have file-loader followed by css-loader. That css-loader can now access the output directory that file-loader is going to use to write its files to. Also non of the unit tests are breaking and they have stayed the same.

It's sending a variable which can be used in other loaders. I don't see how an util directory would help with that as nothing has been written to disk yet?

[alexander-akait]

Here right solution https://github.com/webpack-contrib/file-loader/issues/164#issuecomment-312772005

[cduivis]

No it isn't that change is trying to expose the data to the next loader after file-loader in the "normal" flow. What this PR tries to do is expose that data to loaders which are executed before the file-loader in the normal flow. We do this by making use of the pitching phase to make sure that data can already be accessed.

But I'm really starting to get the feeling that you didn't even read this PR's Motivation / Use Case.

[alexander-akait]

Sorry we can't use this changes, because it is break loader in other cases, we should search other way to provide this information

[alexander-akait]

Please provide minimum reproducible test repo with problem and describe what you try to solve in README, thanks

[cduivis]

You said that before, but as I said can you give me some use cases when it breaks or what is specifically breaking about this ? Because it passes al the unit tests, so what are those breaking changes. If we don't know we can't work on a solution.

[alexander-akait]

@cduivis unit tests !== integration tests, you solution looks very harmful, so i am asking you create example to see what you try to solve

[cduivis]

Please provide minimum reproducible test repo with problem

Unit tests have been provided for this new behavior.

and describe what you try to solve in README, thanks

The intro text of the PR describes this as asked in your contributing guidelines. In there I've placed a link to an repo of extract-loader which is making use of this change There are unit tests in that project as well which you can look into / execute to see what it does.

But I can update the README, which explains how this change could be used in loaders other people make.

[cduivis]

@cduivis unit tests !== integration tests, you solution looks very harmful, so i am asking you create example to see what you try to solve

Could you then provide me those integration tests ? Because looking harmfull !== being harmfull.

[alexander-akait]

The intro text of the PR describes this as asked in your contributing guidelines. In there I've placed a link to an repo of extract-loader which is making use of this change There are unit tests in that project as well which you can look into / execute to see what it does.

But I can update the README, which explains how this change could be used in loaders other people make.

Yes please do this

Could you then provide me those integration tests ? Because looking harmfull !== being harmfull.

Unfortunately no, it is very difficult setup and not for all developers.

After investigate your solution i will have discussion with other developers how to solve this better way

[rjgotten]

@evilebottnawi Sorry we can't use this changes, because it is break loader in other cases, we should search other way to provide this information

I'm going to reiterate what @cduivis asked: which cases?

Because the changes made here should not change pre-existing behavior.

It adds a pitching phase which publishes the desired output URL to all other loaders further into the loader stack, as opposed to the normal loader direction. It places this URL onto the data member specifically meant for transferring data between loaders, as per Webpack's published API documentation.

The changes do not touch the regular loader path. I.e. they do not alter existing behavior. The ONLY danger of an incompatibility here, is the possibility of a name collision with another loader that publishes something in the pitching phase under the same property name.

If that's your gripe; a configuration option can be added to make the pitch behavior opt-in and prevent problems on existing installations.

And yes; some code is being shifted around. I.e. options parsing and URL creation is factored out into re-usable functions, as both the regular loader path as well as the pitching loader path make use of those. Doesn't mean it changes behavior...

[alexander-akait]

Why this PR invalid:

1. Example of problem is not provided
2. PR contains invalid changes like refactor, change version of package and etc (we use aromatically releases)
3. Invalid usage pitch

[rjgotten]

Example of problem not provided

There's already a description at the top of this PR. I assume you want an actual working example to pull down from some repository for demonstration purposes now?

PR contains invalid changes like refactor, change version of package and etc (we use aromatically releases)

The bulk of which were already covered by backing out those changes. A minimum of changes cannot be removed, as your pre-commit hooks and validation with husky were broken and prevented commits to be succesfully pushed up. @cduivis will have more details on that.

Invalid usage pitch

Please explain what's invalid about it, then? Because as far as I can tell; it's not.

Also; we've been using this succesfully from our own fork on internal projects as well. No issues.

So where's the fire? Maybe your integration tests are just flawed? Testing boundaries set more strict than the system requires (and/or the public API dictates) maybe?

Couldn't tell though as you're refusing to publish those integration tests...

[alexander-akait]

I assume you want an actual working example to pull down from some repository for demonstration purposes now?

Yes

The bulk of which were already covered by backing out those changes. A minimum of changes cannot be removed, as your pre-commit hooks and validation with husky were broken and prevented commits to be succesfully pushed up. @cduivis will have more details on that.

Just do rebase and don't touch unnecessary, if you have problem with something please open new issue and it will be fixed in other PR

Please explain what's invalid about it, then? Because as far as I can tell; it's not.

...

As I said above, there are other possibilities of use and they are not covered, I can publish examples later, now I do not have time for this, that is why I ask you to give an example

[cduivis]

Just do rebase and don't touch unnecessary, if you have problem with something please open new issue and it will be fixed in other PR

The rebase is done.

Example of problem is not provided.

If you want to see this change in action please pull down the following repository and run its unit tests extract-loader If you just want to look into the code what it does:

• here is the wepack setup for nested files
• here you can see the unit tests that test if the correct relative paths are generated

As I said above, there are other possibilities of use and they are not covered, I can publish examples later, now I do not have time for this, that is why I ask you to give an example

Please do so, as this back and forth communication is getting us nowhere without well grounded claims. Hopefully they will finally lead us to a mutual understanding.

[alexander-akait]

Thanks for links and feedback, file-loader in my todo

[NormalGaussian]

I could really do with this - is it going to make it in?

[anikethsaha]

I think pitch metho passes data from pitch phase -> normal phase of the same loader !!!

this.callback(.. , .. , .. , data) seems the accurate approach

[rjgotten]

I think pitch metho passes data from pitch phase -> normal phase of the same loader !!!

this.callback(.. , .. , .. , data) seems the accurate approach

A loader has access to this.data which is indeed just a plain object which can hold arbitrary data to share between that loader's pitch and loading phase.

However, a loader also has access to the entire chain of loaders via the this.loaders property and has access to its own index into that array via this.loaderIndex. So, it's completely possible to walk the indices of that array starting from the current loader to the end of the array, to populate earlier loaders -- later in the pitching flow means earlier in the perspective normal flow -- to pre-populate their data and basically "send information further down" which earlier loaders can opt in to using.

That's what this PR does with a bit of path information, so loaders earlier in the chain can use it to resolve a relative path accurately.

---

You're suggesting to use this.callback -- which does something entirely different. When called from the pitching phase: it shorts out the entire loader chain with an early return.

Here's an example: Given a normal loader flow of (in)->A->B->C->D->(out), then the pitching phase executes as D->C->B->A. If C uses this.callback in its pitching phase, then the entire loader chain halts there and returns whatever C returned from the pitching phase, not visiting B and A in the pitching phase and skipping the normal flow A->B->C->D completely.

To put that in ASCII diagrams:

Pitch  Normal

(req)  (resp)
  |      |
  v      ^
  |      |
  D      D
  |      |
  v      ^
  |      |
  C      C
  |      |
  v      ^
  |      |
  B      B
  |      |
  v      ^
  |      |
  A      A
   \    /
   (file)

vs

Pitch  Normal

(req)  (resp)
  |      |
  v      ^
  |      |
  D      |
  |     / 
  v    /  
  |   /   
  C->- this.callback()

[anikethsaha]

this.callback does something entirely different when called from the pitching phase: it shorts out the loader chain, with an early return.

I meant this.callback from normal phase !!

A loader has access to this.data which is indeed just a plain object which can hold arbitrary data to share between that loader's pitch and loading phase.

However, any loader has access to the entire chain of loaders via the this.loaders property and also has access to its own index into that array via this.loaderIndex.

So, it's completely possible to walk the indices of that array starting from the current loader to the > end of the array, to populate earlier loaders -- later inthe pitching flow == earlier in the > perspective normal flow -- to pre-populate their data.

Thanks for this explanation 👍

So, it's completely possible to walk the indices of that array starting from the current loader to the end of the array, to populate earlier loaders -- later inthe pitching flow == earlier in the perspective normal flow -- to pre-populate their data.

Isnt this a hacky workaround ?

[rjgotten]

Isnt this a hacky workaround ?

Yes and no.

From a purely technical implementation perspective it's just accessing publicly reachable data state for another loader. Another loader that hasn't even been reached in the pitching phase yet, so it can't even be running any of its own logic on that data to possibly lead to a race condition. So it's perfectly safe, short of the small odds of there being a naming collision when using string-based properties. There are certainly ways to avoid that as well, by using a Symbol.

But conceptually; yes, it's not very nice. But it's a necessity to cover the blind spot in Webpack's architecture, which only supports bubbling results up in the normal loader flow and does not have an in-built 100% paved cow-path to push metadata down in the pitching flow.

[anikethsaha]

You're suggesting to use this.callback -- which does something entirely different. When called from the pitching phase: it shorts out the entire loader chain with an early return.

Here's an example: Given a normal loader flow of (in)->A->B->C->D->(out), then the pitching phase executes as D->C->B->A. If C uses this.callback in its pitching phase, then the entire loader chain halts there and returns whatever C returned from the pitching phase, not visiting B and A in the pitching phase and skipping the normal flow A->B->C->D completely.

To put that in ASCII diagrams:

Pitch  Normal

(req)  (resp)
  |      |
  v      ^
  |      |
  D      D
  |      |
  v      ^
  |      |
  C      C
  |      |
  v      ^
  |      |
  B      B
  |      |
  v      ^
  |      |
  A      A
   \    /
   (file)

vs

Pitch  Normal

(req)  (resp)
  |      |
  v      ^
  |      |
  D      |
  |     / 
  v    /  
  |   /   
  C->- this.callback()

I meant callback from normal phase !!

I would be something like this

(req)  (resp)
  |        |
  v        ^
  |        |
  D        D
  |        | 
  v        ^
  |        |   
 C       C->- this.callback(.., .., .., data)

And this is the expected behavior if we simply want to send it to the next chained loader

[rjgotten]

I meant callback from normal phase !!

I would be something like this

(req)  (resp)
  |        |
  v        ^
  |        |
  D        D
  |        | 
  v        ^
  |        |   
 C       C->- this.callback(.., .., .., data)

And this is the expected behavior if we simply want to send it to the next chained loader

Using this.callback(.., .., .., data) from C in the normal phase only has D left to pass through. Here, we're talking about data that either B or A needs to know from C, so using this.callback(.., .., .., data) from C in the normal phase is simply too late.

[anikethsaha]

I meant callback from normal phase !! I would be something like this

(req)  (resp)
  |        |
  v        ^
  |        |
  D        D
  |        | 
  v        ^
  |        |   
 C       C->- this.callback(.., .., .., data)

And this is the expected behavior if we simply want to send it to the next chained loader

Using this.callback(.., .., .., data) from C in the normal phase only has D left to pass through. Here, we're talking about data that either B or A needs to know from C, so using this.callback(.., .., .., data) from C in the normal phase is simply too late.

Oh, I thought it was just to pass the data to next loader. This approach through this.data seems fine an unique to file loader and other loaders which moves or writes new file.

@rjgotten thanks a lot for clearing this.

[Yankovsky]

Any news on this issue?

[Yankovsky]

As I see this issue, there are two ways to solve it:

1. Add pitch phase as described by @cduivis and @rjgotten. This approach will make data about path available to all loaders in chain.
2. Use callback https://github.com/webpack-contrib/file-loader/issues/164#issuecomment-312772005 This approach will only provide data about path to next loaders in chain.

Second option is easier to implement, but doesn't cover all possible scenarios. First option has already been implemented in this pr and has only one possible drawback of name collision which could be solved by using Symbol instead of regular string for storing path data.

[Yankovsky]

@evilebottnawi Is there anything unclear about purpose or implementation of this pr? I can help with fixes.

[alexander-akait]

No new feature for file-loader, because we deprecate this loader after webpack@5 stable release in favor https://webpack.js.org/guides/asset-modules/, we will update docs how to achieve same goal - runtime and sources changes, sorry for delay, right now you can copy/paste logic from file-loader, here is no complex logic and we will not change it, sorry for delay

Why it was not merged? Because using pitch can break other loader what rely on this loader. Writing information in loader context is not safe, because it should be immutable, otherwise it will break cache.

[rjgotten]

And when exactly will Webpack 5 be stable, i.e. out of pre-release, and have these asset modules marked as a stable feature that is no longer behind the experiments: true flag? I.e. when will it actually be production-ready for use, with the kinks worked out?

Also; nice to know you have no interest in maintaining this feature on a Webpack 4 branch. I assume that means the entire set of loaders will be end-of-life moving forward to Webpack 5? I.e. no future support for those people and organizations choosing to stay on 4?

[alexander-akait]

@rjgotten

Where I say it? I don't understand why you to talk with me with such a tone?

And when exactly will Webpack 5 be stable, i.e. out of pre-release, and have these asset modules marked as a stable feature that is no longer behind the experiments: true flag? I.e. when will it actually be production-ready for use, with the kinks worked out?

You can already use webpack@5, migration between webpack@4 and webpack@5 will not be complex or hard, most of loaders and plugins already have compatibility and don't have problems.

Asset already stable, we move it to experiments to avoid delay in fixes which can potentially do breaking change. It just means that you will receive fixes and improvements much faster. We don't have an official specification how import file from '/dir/file.js'; should works, so we can do it stable in your understanding. But if you are talking about stability of work - it is stable and covers all the same abilities that are here, in file-loader/url-loader/raw-loader and even with new features.

nice to know you have no interest in maintaining this feature on a Webpack 4 branch

Do you read my answer? https://github.com/webpack-contrib/file-loader/pull/321#issuecomment-676414584

Why it was not merged? Because using pitch can break other loader what rely on this loader. Writing information in loader context is not safe, because it should be immutable, otherwise it will break cache.

I assume that means the entire set of loaders will be end-of-life moving forward to Webpack 5? I.e. no future support for those people and organizations choosing to stay on 4?

Because it has more features in webpack, if you want to stay on webpack@4 you can use this loader all time.