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.

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

Make urls absolute in blob #96

Closed
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
@Strate

Strate commented Dec 3, 2015

Related #93

Strate added some commits Dec 3, 2015

Replace inside blobs relative urls to absolute urls
This avoid requirements to specify absolute path to output.publicPath
@Strate

This comment has been minimized.

Show comment
Hide comment
@Strate

Strate commented Dec 9, 2015

Soo?

@unimonkiez

This comment has been minimized.

Show comment
Hide comment
@unimonkiez

unimonkiez Mar 30, 2016

Give this man a cookie!

unimonkiez commented Mar 30, 2016

Give this man a cookie!

@oller

This comment has been minimized.

Show comment
Hide comment
@oller

oller Mar 30, 2016

Wooo, so does this PR give us full source maps with relative path assets, without the need for an absolute publicPath?

👏

oller commented Mar 30, 2016

Wooo, so does this PR give us full source maps with relative path assets, without the need for an absolute publicPath?

👏

@Strate

This comment has been minimized.

Show comment
Hide comment
@Strate

Strate Mar 30, 2016

@oller sure. Tested by using in real project, works like a charm.

Strate commented Mar 30, 2016

@oller sure. Tested by using in real project, works like a charm.

@sergeylukin

This comment has been minimized.

Show comment
Hide comment
@sergeylukin

sergeylukin Apr 4, 2016

Worked for me at first glance. Any reason why this isn't merged?
@Strate I suggest squashing commits into one

sergeylukin commented Apr 4, 2016

Worked for me at first glance. Any reason why this isn't merged?
@Strate I suggest squashing commits into one

@adam-stanek

This comment has been minimized.

Show comment
Hide comment
@adam-stanek

adam-stanek Apr 7, 2016

Thanks for the patch. Works like charm.

adam-stanek commented Apr 7, 2016

Thanks for the patch. Works like charm.

@Strate

This comment has been minimized.

Show comment
Hide comment
@Strate

Strate Apr 7, 2016

@sokra this is really helpful patch, as you can see. could you merge it? Or maybe I could somehow improve this patch?

Strate commented Apr 7, 2016

@sokra this is really helpful patch, as you can see. could you merge it? Or maybe I could somehow improve this patch?

@geekyme

This comment has been minimized.

Show comment
Hide comment
@geekyme

geekyme Apr 28, 2016

bump. Need this patch

geekyme commented Apr 28, 2016

bump. Need this patch

@vperron

This comment has been minimized.

Show comment
Hide comment
@vperron

vperron May 10, 2016

Come on, please merge :)

vperron commented May 10, 2016

Come on, please merge :)

@oller

This comment has been minimized.

Show comment
Hide comment
@oller

oller May 11, 2016

Pleeeeaaase! 🎁

oller commented May 11, 2016

Pleeeeaaase! 🎁

@tizmagik

This comment has been minimized.

Show comment
Hide comment
@tizmagik

tizmagik May 11, 2016

It's been 5 months... Is there any reason for this not to be merged @sokra ?

tizmagik commented May 11, 2016

It's been 5 months... Is there any reason for this not to be merged @sokra ?

@sokra

This comment has been minimized.

Show comment
Hide comment
@sokra

sokra May 11, 2016

Member

yes. This is not a good solution. It should be solved on CSS AST. It's better to solve it before the style loader in the css loader

Member

sokra commented May 11, 2016

yes. This is not a good solution. It should be solved on CSS AST. It's better to solve it before the style loader in the css loader

@codisms

This comment has been minimized.

Show comment
Hide comment
@codisms

codisms May 11, 2016

@sokra I can't find any code in css-loader that is client side, which is where this logic would need to reside so it knows the browser location at run time. Can you point us in the right direction?

codisms commented May 11, 2016

@sokra I can't find any code in css-loader that is client side, which is where this logic would need to reside so it knows the browser location at run time. Can you point us in the right direction?

@bendytree

This comment has been minimized.

Show comment
Hide comment
@bendytree

bendytree May 20, 2016

@Strate great idea and great work!

I just submitted a very similar pull request which is opt-in via a style?fixUrls flag and has some unit tests to verify the behavior since I'm not parsing the CSS into an AST.

Anyone feel free to fork bendytree/style-loader and then use a git url in your package.json:

...
"style-loader": "git+https://git@github.com/YOUR_USERNAME/style-loader.git",
...

bendytree commented May 20, 2016

@Strate great idea and great work!

I just submitted a very similar pull request which is opt-in via a style?fixUrls flag and has some unit tests to verify the behavior since I'm not parsing the CSS into an AST.

Anyone feel free to fork bendytree/style-loader and then use a git url in your package.json:

...
"style-loader": "git+https://git@github.com/YOUR_USERNAME/style-loader.git",
...
@tizmagik

This comment has been minimized.

Show comment
Hide comment
@tizmagik

tizmagik Jun 20, 2016

@sokra any guidance you could provide @Strate / @bendytree et al to solve this properly in the CSS AST as you suggested?

tizmagik commented Jun 20, 2016

@sokra any guidance you could provide @Strate / @bendytree et al to solve this properly in the CSS AST as you suggested?

@Strate

This comment has been minimized.

Show comment
Hide comment
@Strate

Strate Jun 20, 2016

As for me, use css ast on client side would be an overkill. This patch used in real world project since it's creation without any problem. With keeping in mind that it used only in dev environment, I could say that it has sufficient quality to merge.

Strate commented Jun 20, 2016

As for me, use css ast on client side would be an overkill. This patch used in real world project since it's creation without any problem. With keeping in mind that it used only in dev environment, I could say that it has sufficient quality to merge.

@SpearThruster

This comment has been minimized.

Show comment
Hide comment
@SpearThruster

SpearThruster Jun 22, 2016

This is a huge issue on my project as well, can we get update why this hasn't been merged?

SpearThruster commented Jun 22, 2016

This is a huge issue on my project as well, can we get update why this hasn't been merged?

@motleydev

This comment has been minimized.

Show comment
Hide comment
@motleydev

motleydev Jun 23, 2016

Adding an additional "please". Or at least provide feedback how this could be made better.

motleydev commented Jun 23, 2016

Adding an additional "please". Or at least provide feedback how this could be made better.

@SimonSelg

This comment has been minimized.

Show comment
Hide comment
@SimonSelg

SimonSelg Jun 23, 2016

I'll work on this this afternoon. I have some ideas how to solve this correctly on AST level. But I'll have to test the performance impact.

SimonSelg commented Jun 23, 2016

I'll work on this this afternoon. I have some ideas how to solve this correctly on AST level. But I'll have to test the performance impact.

@Strate

This comment has been minimized.

Show comment
Hide comment
@Strate

Strate Jun 24, 2016

Attention please.

I've found a solution for the problem without patching this loader.
Just add following line on the top of your topmost entry does the job:

__webpack_public_path__ = window.location.protocol + "//" + window.location.host + "/"

This line modifies output.publicPath option at browser runtime, adding current protocol and host. If you have a custom publicPath, just add it to the end of prefix.
This solution requires you to have no output.publicPath specified in config or command line.
If you need to pass publicPath from command line or webpack configuration file you can use environment variables together with DefinePlugin:

// webpack.config.js

module.exports = {
  pluginns: [
    new webpack.DefinePlugin({
      'process.env.WEBPACK_PUBLIC_PATH': JSON.stringify(process.env.WEBPACK_PUBLIC_PATH)
    })
  ]
}

// topmostEntryPoint.js
__webpack_public_path__ = window.location.protocol + "//" + window.location.host + "/" + process.env.WEBPACK_PUBLIC_PATH;

and run build with command:

WEBPACK_PUBLIC_PATH="your/public/path/" webpack

Strate commented Jun 24, 2016

Attention please.

I've found a solution for the problem without patching this loader.
Just add following line on the top of your topmost entry does the job:

__webpack_public_path__ = window.location.protocol + "//" + window.location.host + "/"

This line modifies output.publicPath option at browser runtime, adding current protocol and host. If you have a custom publicPath, just add it to the end of prefix.
This solution requires you to have no output.publicPath specified in config or command line.
If you need to pass publicPath from command line or webpack configuration file you can use environment variables together with DefinePlugin:

// webpack.config.js

module.exports = {
  pluginns: [
    new webpack.DefinePlugin({
      'process.env.WEBPACK_PUBLIC_PATH': JSON.stringify(process.env.WEBPACK_PUBLIC_PATH)
    })
  ]
}

// topmostEntryPoint.js
__webpack_public_path__ = window.location.protocol + "//" + window.location.host + "/" + process.env.WEBPACK_PUBLIC_PATH;

and run build with command:

WEBPACK_PUBLIC_PATH="your/public/path/" webpack
@IAMtheIAM

This comment has been minimized.

Show comment
Hide comment
@IAMtheIAM

IAMtheIAM Aug 19, 2016

@Strate Thank you! This did the trick for me to get the paths to resolve correctly inside my SCSS file. I used it for loading fonts in scss, when the scss file is a "blob://" rather than an inline style.

// topmostEntryPoint.js
__webpack_public_path__ = window.location.protocol + "//" + window.location.host + "/" + process.env.WEBPACK_PUBLIC_PATH;
require('./index.style.scss');

IAMtheIAM commented Aug 19, 2016

@Strate Thank you! This did the trick for me to get the paths to resolve correctly inside my SCSS file. I used it for loading fonts in scss, when the scss file is a "blob://" rather than an inline style.

// topmostEntryPoint.js
__webpack_public_path__ = window.location.protocol + "//" + window.location.host + "/" + process.env.WEBPACK_PUBLIC_PATH;
require('./index.style.scss');

@kopax kopax referenced this pull request Oct 16, 2016

Closed

How to enable source map in dev with chrome #1112

0 of 1 task complete

d4rkr00t added a commit to d4rkr00t/aik that referenced this pull request Nov 17, 2016

fix(webpack): Images are not being loaded using webpack-dev-server
Ugly hack which solves an issue with not loading images with generated urls in css files. Related

issues: webpack-contrib/style-loader#96

@michael-ciniawsky michael-ciniawsky self-assigned this Mar 6, 2017

@bebraw

Needs rebase + tests.

@michael-ciniawsky

yep rebase and test please

@d3viant0ne

This comment has been minimized.

Show comment
Hide comment
@d3viant0ne

d3viant0ne Mar 6, 2017

Member

Also - There were compliance issues we needed to clear up before the CLA bot could be enabled, that was cleared up about 10 days ago.

To get the CLA signed, you just need to close this PR & open it again. That will trigger the CLA workflow again which should allow you to sign it.

Please note, before you close & open it again ensure the email in your local git config matches what you have configured in github.

Member

d3viant0ne commented Mar 6, 2017

Also - There were compliance issues we needed to clear up before the CLA bot could be enabled, that was cleared up about 10 days ago.

To get the CLA signed, you just need to close this PR & open it again. That will trigger the CLA workflow again which should allow you to sign it.

Please note, before you close & open it again ensure the email in your local git config matches what you have configured in github.

@ekulabuhov

This comment has been minimized.

Show comment
Hide comment
@ekulabuhov

ekulabuhov Mar 6, 2017

Member

I looks like this PR might be superseded by #124?

Member

ekulabuhov commented Mar 6, 2017

I looks like this PR might be superseded by #124?

@d3viant0ne

This comment has been minimized.

Show comment
Hide comment
@d3viant0ne

d3viant0ne Mar 6, 2017

Member

Yes. My take on all of this would be to land #124, put #165 in the next major & close #96 but that plan needs to be confirmed by the @webpack-contrib/org-maintainers

Member

d3viant0ne commented Mar 6, 2017

Yes. My take on all of this would be to land #124, put #165 in the next major & close #96 but that plan needs to be confirmed by the @webpack-contrib/org-maintainers

@bebraw

This comment has been minimized.

Show comment
Hide comment
@bebraw

bebraw Mar 6, 2017

Contributor

@d3viant0ne Yeah, sounds fine.

Contributor

bebraw commented Mar 6, 2017

@d3viant0ne Yeah, sounds fine.

@Strate

This comment has been minimized.

Show comment
Hide comment
@Strate

Strate Mar 6, 2017

@d3viant0ne have you read this comment: #96 (comment)? This is how I solve this issue current in my project. I think it is enougth to move this trick to README.

Strate commented Mar 6, 2017

@d3viant0ne have you read this comment: #96 (comment)? This is how I solve this issue current in my project. I think it is enougth to move this trick to README.

@d3viant0ne

This comment has been minimized.

Show comment
Hide comment
@d3viant0ne

d3viant0ne Mar 6, 2017

Member

It's workaround vs. fix where the workaround is only effective in some cases. This is an issue that needs to be resolved in code

Member

d3viant0ne commented Mar 6, 2017

It's workaround vs. fix where the workaround is only effective in some cases. This is an issue that needs to be resolved in code

@michael-ciniawsky

This comment has been minimized.

Show comment
Hide comment
@michael-ciniawsky

michael-ciniawsky Mar 17, 2017

Member

#186 (#124) landed, is this the PR still relevant then? If @Strate please close and reopen to trigger the CLA Bot again and rebase against current master, otherwise if you lost interest in this PR in general / #186 (#124) solved the issue please confirm we can close

Member

michael-ciniawsky commented Mar 17, 2017

#186 (#124) landed, is this the PR still relevant then? If @Strate please close and reopen to trigger the CLA Bot again and rebase against current master, otherwise if you lost interest in this PR in general / #186 (#124) solved the issue please confirm we can close

@d3viant0ne

This comment has been minimized.

Show comment
Hide comment
@d3viant0ne

d3viant0ne Mar 17, 2017

Member

screen shot 2017-03-16 at 8 38 29 pm

This should be closed in favor of #124 which has already been merged.

Member

d3viant0ne commented Mar 17, 2017

screen shot 2017-03-16 at 8 38 29 pm

This should be closed in favor of #124 which has already been merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment