Skip to content
This repository has been archived by the owner on Mar 17, 2021. It is now read-only.

Allow publicPath handler to return a function instead of string #327

Closed
wants to merge 3 commits into from

Conversation

prontiol
Copy link

This PR contains a:

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

Motivation / Use-Case

Sometimes you might need to construct the path to your assets in runtime (for example when using multi CDN for different types of assets). Currently the only way to change the base path is to update __webpack_public_path__ and it will affect all the assets at once 馃槥

If only we could have a function instead of plain string which will be executed in runtime and return the path based on the asset type, variables, cookies or whatever you want to figure out the right base path... oh wait, but we can!
This PR is to rescue! 馃帀

Breaking Changes

No breaking changes.
Current implementation just produces nothing If you return a function instead of string out of the options.publicPath handler because JSON.stringify(function) === undefined
This PR enhances file-loader and now you can return a function which will be executed in runtime so you can enjoy all your variables and helper functions in it and construct a publicPath of your dream.

@jsf-clabot
Copy link

jsf-clabot commented Apr 23, 2019

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

Looks good, just the question - how you want to solve problem for other loader? I think we can solve this problem on webpack level? Can you provide simple example?

@prontiol
Copy link
Author

@evilebottnawi Can you elaborate the question?

@alexander-akait
Copy link
Member

how you want to solve problem for other loader?

Because it is fix only logic for file-loader, but other loader also can emit assets, i think we need solve this on webpack side, so i request minimum reproducible test repo

@prontiol
Copy link
Author

Well, I am not sure this can be solved on webpack level.
At least this is not something I am willing to fix.

@alexander-akait
Copy link
Member

We don't want add new option to file-loader, because in webpack@5 it was built-in loader, so we need solve this problem on webpack level to avoid same problems in future for other developers

@prontiol
Copy link
Author

Okay no problem, we will just fork it.

@alexander-akait
Copy link
Member

alexander-akait commented Apr 23, 2019

@prontiol why is hard to help other developers solve same problem? Is it hard to provide minimum reproducible test repo? Maybe we can same logic without modify source code

@prontiol
Copy link
Author

It's not hard so I made this PR. It fixes the problem I encountered and can help others.
You said you didn't want to add new options to file-loader because of webpack 5 so I assume this PR won't be merged, that's why we will fork it because we need this fixed now and we can't wait for webpack 5. And I am not sure webpack 5 can do this either.
Generally I didn't expect this resistance as this PR doesn't introduce any breaking changes.
It just handles function output of publicPath in a right way.

Regarding the minimum reproducible repo: test case included in the PR is pretty much self-explanatory.

@alexander-akait
Copy link
Member

Stop be toxic. I am open to feedback, unlike how you build a dialogue.

You said you didn't want to add new options to file-loader because of webpack 5 so I assume this PR won't be merged, that's why we will fork it because we need this fixed now and we can't wait for webpack 5.

I said that I would not like to add, but if we do not find other solutions, it will be merged

that's why we will fork it because we need this fixed now and we can't wait for webpack 5

Yes, but we need feedback from developers to solve problems and design right architecture to avoid problems in future (for loader and for webpack@5).

Generally I didn't expect this resistance as this PR doesn't introduce any breaking changes.
It just handles function output of publicPath in a right way.

This does not mean that we should merge all PRs that are sent to us just because they are not breaking change

Regarding the minimum reproducible repo: test case included in the PR is pretty much self-explanatory.

No, maybe you use invalid approach to storage or organization assets, maybe you need other setup for cdn, and many many cases in other way

What i want? I want example where standard functionality not solve you problem with real life example. This will allow us to think how best to solve such problems now and in the future.

@prontiol
Copy link
Author

I am no longer interested in contributing to this repo.

@prontiol prontiol closed this Apr 24, 2019
@prontiol prontiol mentioned this pull request Mar 2, 2020
6 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants