feat(index): add `cssOutputPath` option (`option.cssOutputPath`) #150

adriancmiranda · 2017-04-13T05:20:00Z

What kind of change does this PR introduce?

A bugfix and a feature (cssOutputPath)

Did you add tests for your changes?
no

If relevant, did you update the README?
The readme file has been updated

Summary

#149 and #135 public path validation

Does this PR introduce a breaking change?

no

Other information
Entry files and CSS output path doesn't pass through the file-loader so we haven't access to the file's context to compare with your issuer context correctly then we need to kidnap javascript entries and manually pass the CSS file output path using a new property (i.e. cssOutputPath). This should prevent unexpected output paths.

bebraw · 2017-04-14T03:52:32Z

.gitignore

 !.gitignore
+!.travis.yml


Why to change these two?

Yep these will be taken care of by #130

I ignored all dotfiles in my last commit, but I forgot to allow .travis.yml too, not only the .gitignore.
About the .editorconfig it was my carelessness shouldn't be there yet (what to led change formatting), I've been working on another machine and it would configure wrong. Sorry!

bebraw · 2017-04-14T03:53:54Z

index.js

@@ -1,80 +1,106 @@
 /*
-	MIT License http://www.opensource.org/licenses/mit-license.php
-	Author Tobias Koppers @sokra
+  MIT License http://www.opensource.org/licenses/mit-license.php


The formatting change should probably be a separate PR.

npm i -D jsdoc-to-markdown

package.json

{ "scripts": { "docs": "jsdoc2md index.js > LOADER.md" } }

index.js

var path = require('path') ... /** * File Loader * * > Loads and emits files * * @author Tobias Koppers (@sokra) * @version 0.12.0 * @license MIT * * @module file-loader * * @requires path * @requires loaderUtils * * @method loader * * @param {String} file File * * @return {String} module Module (File) */ module.exports = function (file) {...}

@bebraw I agree. I'll change this.
@michael-ciniawsky I don't know if I can change the header. I see a pattern in other plugins also can't occur a conflict with #130 later?

michael-ciniawsky · 2017-04-14T04:38:21Z

.gitignore

 !.gitignore
+!.travis.yml


Yep these will be taken care of by #130

michael-ciniawsky · 2017-04-14T04:46:41Z

index.js

@@ -1,80 +1,106 @@
 /*
-	MIT License http://www.opensource.org/licenses/mit-license.php
-	Author Tobias Koppers @sokra
+  MIT License http://www.opensource.org/licenses/mit-license.php


npm i -D jsdoc-to-markdown

package.json

{ "scripts": { "docs": "jsdoc2md index.js > LOADER.md" } }

index.js

var path = require('path') ... /** * File Loader * * > Loads and emits files * * @author Tobias Koppers (@sokra) * @version 0.12.0 * @license MIT * * @module file-loader * * @requires path * @requires loaderUtils * * @method loader * * @param {String} file File * * @return {String} module Module (File) */ module.exports = function (file) {...}

michael-ciniawsky · 2017-04-14T04:48:22Z

index.js

-	};
+  var query = loaderUtils.getOptions(this) || {};
+  var configKey = query.config || "fileLoader";
+  var options = this.options[configKey] || {};


General Question: Is this still needed 😛 ? What does this.options contain ? 🙃 (webpack.config.js) ?

That was already there and I preferred to keep it as it was to avoid breaks.
About the question, yes! this.options is from webpack.config.js 🙃

michael-ciniawsky · 2017-04-14T04:49:08Z

index.js

+  var query = loaderUtils.getOptions(this) || {};
+  var configKey = query.config || "fileLoader";
+  var options = this.options[configKey] || {};
+  var config = {


config => defaults ?

Yeap! But textOutputPath I kept as undefined then I don't think to be necessary there, just as the regExp option isn't. What do you think?

Sry I didn't understand before, I thought it was part of the review ^^

michael-ciniawsky · 2017-04-14T04:49:36Z

index.js

-		config[attr] = options[attr];
-	});
+  // options takes precedence over config
+  Object.keys(options).forEach(function(attr) {


Object.assign?

That was there already, I believe it's to support older versions of node.

kk sry leave it :) I will test it when time and it's separate PR

Stick a todo in there to switch this to Object.assign in the defaults pull request which removes support for older node version.

michael-ciniawsky · 2017-04-14T04:50:17Z

index.js

-		config[attr] = query[attr];
-	});
+  // query takes precedence over config and options
+  Object.keys(query).forEach(function(attr) {


Object.assign? Why is that done twice ?

That was there already too, all these changes is polluted by whitespace simply add ?w=1 at the end of the URI.

michael-ciniawsky · 2017-04-14T04:53:09Z

index.js

-		);
-	}
+    // We have access only to entry point relationships. So we work with this relations.
+    var issuerContext = (this._module && this._module.issuer && this._module.issuer.context) || context;


const issuer = {...} const relative = { url: '...' , path: '...' } const output = {...}

I'm sorry, I didn't understand these review.

It makes sense for me, but I also think it's necessary another PR

Nope not really 😛 , but it's ok to leave it for now

@michael-ciniawsky So, let's do it! 🙃

EDIT
Something like this?
https://gist.github.com/adriancmiranda/43478d0ad1b8402b412d5c56ef3dd57f

In the gist a newline \n after the follwing lines and I'm 👍 on Code Style 😛 L50, 65, 74, 84

@adriancmiranda Update this PR with the Gist, we will squash it later anyways

michael-ciniawsky · 2017-04-14T05:03:18Z

index.js

+    if (output.filename && output.filename !== "extract-text-webpack-plugin-output-filename") {
+      relativePath = outputDirname;
+    } else if (toString.call(config.textOutputPath) === "[object String]") {
+      var outputPackageDirname = output.path.replace(this.options.context + path.sep, "");


See the comment directly above, please group issuer, relative, output via an {Object}

michael-ciniawsky · 2017-04-14T05:10:46Z

Is this in general 'only' related/compatible to extract-text-webpack-plugin, or could this be used by other plugins etc aswell? assetsOutputPath || assetsPath

adriancmiranda

extract-text-webpack-plugin-output-filename is sent for all requested files (except js entries) by the specific plugin, but the output.filename is from webpack.config then I don't think that's a problem to use with another plugin unless it also sends something like another-plugin-output-filename for all requested files.

The textOutputPath is relative to the extracted file (e.g css), is from the issuer, not for your assets. I'm not sure if the extract-text(for example) is used only for CSS files so I prefer to keep the generic name or maybe cssOutputPath. I don't know. What do you think?

adriancmiranda · 2017-04-14T08:04:58Z

.gitignore

 !.gitignore
+!.travis.yml


I ignored all dotfiles in my last commit, but I forgot to allow .travis.yml too, not only the .gitignore.
About the .editorconfig it was my carelessness shouldn't be there yet (what to led change formatting), I've been working on another machine and it would configure wrong. Sorry!

adriancmiranda · 2017-04-14T08:13:28Z

index.js

-	};
+  var query = loaderUtils.getOptions(this) || {};
+  var configKey = query.config || "fileLoader";
+  var options = this.options[configKey] || {};


That was already there and I preferred to keep it as it was to avoid breaks.
About the question, yes! this.options is from webpack.config.js 🙃

adriancmiranda · 2017-04-14T08:22:49Z

index.js

+  var query = loaderUtils.getOptions(this) || {};
+  var configKey = query.config || "fileLoader";
+  var options = this.options[configKey] || {};
+  var config = {


Yeap! But textOutputPath I kept as undefined then I don't think to be necessary there, just as the regExp option isn't. What do you think?

adriancmiranda · 2017-04-14T08:27:10Z

index.js

-		config[attr] = options[attr];
-	});
+  // options takes precedence over config
+  Object.keys(options).forEach(function(attr) {


That was there already, I believe it's to support older versions of node.

adriancmiranda · 2017-04-14T08:29:01Z

index.js

-		config[attr] = query[attr];
-	});
+  // query takes precedence over config and options
+  Object.keys(query).forEach(function(attr) {


That was there already too, all these changes is polluted by whitespace simply add ?w=1 at the end of the URI.

adriancmiranda · 2017-04-14T08:39:52Z

index.js

-		);
-	}
+    // We have access only to entry point relationships. So we work with this relations.
+    var issuerContext = (this._module && this._module.issuer && this._module.issuer.context) || context;


I'm sorry, I didn't understand these review.

michael-ciniawsky · 2017-04-14T20:02:24Z

extract-text-webpack-plugin-output-filename is sent for all requested files (except js entries) by the specific plugin, but the output.filename is from webpack.config then I don't think that's a problem to use with another plugin unless it also sends something like another-plugin-output-filename for all requested files.

kk, yeah this might be out-of-scope of this PR, I'm sry to bikeshed about this, but we need to make sure to not let any specific hooks relying on other plugins/loaders into loaders/plugins in general when possible and try go for the generic solution first. So if we can design it as a generic hook for plugins in need of the same functionality as ETWP, we should do it.

But this doesn't mean it's a blocker for this PR :), tbh I have no clue if any other plugin would need this aswell. I just need to clearify this 😛

The textOutputPath is relative to the extracted file (e.g css), is from the issuer, not for your assets. I'm not sure if the extract-text(for example) is used only for CSS files so I prefer to keep the generic name or maybe cssOutputPath. I don't know. What do you think?

If we go with this, we need to make sure folks using the file-loader+ ETWP combo, directly see this option is for you 😛

textOutput[Path] (Current)
extractOutput[Path]
extractTextOutput[Path]

adriancmiranda · 2017-04-15T00:41:26Z

@michael-ciniawsky No, you're totally right and I'm happy to answer.
Actually, my idea is quite simple and maybe that's exactly why it's confusing without seeing it in action.
At this line, I expected only JavaScript files and I filled it with this info because it has only the part of the relative path that matters i.e without any ../ because it's generally requested from the root. So actually this line is to prevent that plugin to overrides any path with extract-text-webpack-plugin-output-filename so, isn't exactly a dependency from ETWP, but a dependency from JS entry as an issuer context. Already in this line I'm particularly expecting only CSS files but nothing prevent it from being another kind of file (the only reason I called from textOutputPath and not cssOutputPath), again, you're right, this name generates a mess and my quotations for the plugin enforce this too, but how can we call it?
I vote in cssOutputPath, but I don't know if it right why I'm not sure if it will be used only for CSS files.

EDIT
Forget about it. I removed the references to ETWP
and seeing some options, I'll change textOutputPath to cssOutputPath to simplify the usage.

michael-ciniawsky · 2017-04-15T03:13:07Z

index.js

 		var output = this.options.output || {};
-		if (output.filename && output.filename !== "extract-text-webpack-plugin-output-filename") {
+		if (output.filename && path.extname(output.filename) === ".js") {


It's wrong yet. I remembered, it has 'jsx' too, maybe another kind of file for scripts. Maybe we need only to know if the file has an extension.

joshwiens · 2017-06-06T10:28:26Z

@adriancmiranda - - I have set this to blocked ( #165 ) & moved this feature into 1.1.0, the next minor range after the pending 1.0.0 release.

adriancmiranda · 2017-06-06T13:22:35Z

@d3viant0ne Thank you... and I'm sorry for the noise 😉

codecov · 2017-06-08T20:42:08Z

Codecov Report

Merging #150 into master will increase coverage by 0.72%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #150      +/-   ##
==========================================
+ Coverage   97.14%   97.87%   +0.72%     
==========================================
  Files           2        3       +1     
  Lines          35       47      +12     
  Branches       16       20       +4     
==========================================
+ Hits           34       46      +12     
  Misses          1        1

Impacted Files	Coverage Δ
src/helper.js	`100% <100%> (ø)`
src/index.js	`100% <100%> (+3.12%)`	⬆️
src/cjs.js	`0% <0%> (-100%)`	⬇️

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 0c4cdc2...bf65289. Read the comment docs.

stereokai · 2017-10-16T08:56:06Z

@adriancmiranda @michael-ciniawsky @d3viant0ne

What about this solution? Will it ever be merged? Half of my images end up in the parent folder. Also with all politeness (and seriousness) I think the priority is a lot higher than "nice to have". Thanks for considering!

adriancmiranda · 2017-10-16T16:30:21Z

Hey @stereokai I'm waiting for another review yet. 😉

EDIT: Perhaps It will take some time, you can use the fork from @woshi82 for now, if you wish to try out this version (there are a compiled version).

stereokai · 2017-10-25T11:49:20Z

@adriancmiranda Actually, here's an update:

After 2 days of trial and error (Thanks Webpack docs!) I found out that there's a way to control file-loader's output and replicate the file structure of the source directory in the output directory. It's the Webpack setting context, which can be used per loader or per build, For file-loader:

  use: [{
    loader: 'file-loader',
    options: {
      context: <path>, // The directory to draw relative paths from
      name: '[path][name].[ext]'
    },
  },

Here's an example of how to use it. Let's say your desired result is to load an image via www.website.com/assets/images/user.png

Your project's file structure is:

project/
├── src/
│   └── assets/
│       └── images/
│           └── user.png
└── build/

And the desired result is:

project/
├── src/
│   └── assets/
│       └── images/
│           └── user.png
└── build/
    └── assets/
        └── images/
            └── user.png

The configuration for this is:

  use: [{
    loader: 'file-loader',
    options: {
      context: path.resolve(__dirname, 'src')
      name: '[path][name].[ext]'
    },
  },

And that is because you want to replicate the file structure under src folder inside the build folder.

If you wanted to remove the assets directory from the path, the value for context would be: path.resolve(__dirname, 'src/assets'), and file-loader will replicate the relative paths starting in 'src/assets' instead, and the result would be:

project/
├── src/
│   └── assets/
│       └── images/
│           └── user.png
└── build/
    └── images/
        └── user.png

adriancmiranda · 2017-10-25T14:38:33Z

@stereokai How about the links?
Just to clarify. This PR is to keep the relative paths in your build. 😋

stereokai · 2017-10-26T07:31:45Z

@adriancmiranda what do you mean? I was getting build assets in parent folders of the project root. Are we not talking about the same problem?

adriancmiranda · 2017-11-27T03:04:44Z

@stereokai What I'm proposing isn't exactly solving a problem from file-loader (file-loader is fine), but provide an additional solution to, so that you can also have relative paths in the compiled version of your code. Your example works perfectly, however, it only has one file-per-configuration context. So let's say you have this structure:

src/
│   └── scripts/index.js -> require('../assets/images/placehold.jpg') 
│   └── assets/
│       └── images/

you get something like this, right?

/src/assets/images/placehold.jpg

My proposal is that you have a result like this:

../assets/images/placehold.jpg

considering of course that you'll replicate src dir. But let's say you want to change the build path from the js file to the root, something like this.

build/
│   └── index.js -> require('./assets/images/placehold.jpg') 
│   └── assets/
│       └── images/

In this case you should have this path

assets/images/placehold.jpg

instead

/assets/images/placehold.jpg

P.S: Here is some tests extracted from here to you play with this.

stereokai · 2017-11-27T09:46:40Z

@adriancmiranda Does this line: scripts/index.js -> require('../assets/images/placehold.jpg') mean scripts/index.js is importing placehold.jpg?

adriancmiranda · 2017-11-27T12:43:45Z

@stereokai Yes, it is, sorry about it. I wasn't clear

stereokai · 2017-11-28T13:06:15Z

@adriancmiranda I'm sorry but I don't understand what it is you are trying to achieve

adriancmiranda · 2018-01-10T17:08:53Z

@stereokai If you are curious about this, I recommend to you test this version. I would like to hear your feedback 😉

Leward · 2018-03-08T17:55:04Z

The solution worked on my project (Cordova project so I cannot use / as public path)

In my package.json I put:

"file-loader": "git://github.com/woshi82/file-loader.git#master"

In my webpack config:

modules: {
  rules: [
    // ...
    {
        test: /\.(png|jpe?g|gif|svg)(\?.*)?$/,
        loader: 'url-loader',
        options: {
          limit: 5000, 
          name: utils.assetsPath('img/[name].[hash:7].[ext]'),
          useRelativePath: process.env.NODE_ENV === "production",
          cssOutputPath: 'static/css'
        }
      }
  ]
}

The piece of CSS I am using:

select {
  background: url('../assets/select-arrow.png') no-repeat right transparent;
}

The generated CSS rule:

select {
    background: url(../../assets/static/img/select-arrow.8493640.png) no-repeat 100% transparent;
}

adriancmiranda · 2018-03-08T20:11:44Z

@Leward The useRelativePath should works on development mode too :-)
P.S.: Thank you for your feedback 😉

michael-ciniawsky

I don't think we should add any type specific (css) options to this loader and the orginial problem definitely lied in extract-text-webpack-plugin, which shouldn't affect a simple loader like the file-loader

I'm going to close this PR for now, but feel free to reopen if in disagreement and to reiterate

adriancmiranda added 12 commits April 11, 2017 21:15

fix: outputs from useRelativePath

a92f630

fix: Allowing .travis.yml file in git

e15e3a4

style: indentation

6c5f129

fix: outputs

509855d

style: keeping only double quotes

1eb69cd

fix: textOutputPath parameter validation

7dbce89

fix: normalized outputPath sep in windows os

ffe9709

docs: textOutputPath parameter

f2ebd86

docs: textOutputPath parameter

d3ddff5

docs: textOutputPath parameter

7cf4921

style: keeping only double quotes

74c9e98

fix: public path from js files

5c4d2a4

adriancmiranda changed the title ~~fix: useRelativePath~~ fix: useRelativePath outputs Apr 13, 2017

adriancmiranda changed the title ~~fix: useRelativePath outputs~~ fix: useRelativePath Apr 13, 2017

joshwiens requested review from bebraw, michael-ciniawsky and joshwiens April 14, 2017 02:23

joshwiens added the pr: review label Apr 14, 2017

bebraw reviewed Apr 14, 2017

View reviewed changes

michael-ciniawsky suggested changes Apr 14, 2017

View reviewed changes

adriancmiranda commented Apr 14, 2017

View reviewed changes

style: identation to tabs

bdbb0e2

adriancmiranda added 4 commits April 14, 2017 23:40

chore: removed vendor references

39b66b2

chore: changing textOutputPath to cssOutputPath

4d1a43c

docs: changing textOutputPath to cssOutputPath

3dde284

fix: cssOutputPath defaults

584f009

michael-ciniawsky reviewed Apr 15, 2017

View reviewed changes

Merge branch 'master' into master

1cf6f67

feat: add option.cssOutputPath

bf65289

joshwiens removed the status: Blocked label Jun 10, 2017

michael-ciniawsky added priority: 5 (nice to have) status: Pending labels Jul 28, 2017

weaverryan mentioned this pull request Oct 12, 2017

Fix issue incorrect paths for fonts. Add methods to Use relative paths symfony/webpack-encore#179

Open

michael-ciniawsky changed the title ~~feat: add option.cssOutputPath~~ feat(index): add cssOutputPath option (option.cssOutputPath) Aug 21, 2018

michael-ciniawsky suggested changes Aug 21, 2018

View reviewed changes

michael-ciniawsky closed this Aug 21, 2018

michael-ciniawsky added status: Declined and removed pr_action: Rework priority: 5 (nice to have) status: Pending labels Aug 21, 2018

michael-ciniawsky removed this from the 1.2.0 milestone Aug 21, 2018

michael-ciniawsky removed their assignment Aug 21, 2018

		!.gitignore
		!.travis.yml

		!.gitignore
		!.travis.yml

		!.gitignore
		!.travis.yml

Uh oh!

feat(index): add cssOutputPath option (option.cssOutputPath) #150

feat(index): add cssOutputPath option (option.cssOutputPath) #150

Uh oh!

Conversation

adriancmiranda commented Apr 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michael-ciniawsky Apr 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adriancmiranda Apr 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adriancmiranda Apr 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michael-ciniawsky Apr 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michael-ciniawsky Apr 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adriancmiranda Apr 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michael-ciniawsky Apr 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adriancmiranda Apr 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michael-ciniawsky Apr 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

feat(index): add `cssOutputPath` option (`option.cssOutputPath`) #150

feat(index): add `cssOutputPath` option (`option.cssOutputPath`) #150

adriancmiranda commented Apr 13, 2017 •

edited

Loading

michael-ciniawsky Apr 14, 2017 •

edited

Loading

adriancmiranda Apr 14, 2017 •

edited

Loading

adriancmiranda Apr 14, 2017 •

edited

Loading

michael-ciniawsky Apr 14, 2017 •

edited

Loading

michael-ciniawsky Apr 14, 2017 •

edited

Loading

adriancmiranda Apr 14, 2017 •

edited

Loading

michael-ciniawsky Apr 14, 2017 •

edited

Loading

adriancmiranda Apr 14, 2017 •

edited

Loading

michael-ciniawsky Apr 14, 2017 •

edited

Loading

adriancmiranda Apr 15, 2017 •

edited

Loading

michael-ciniawsky commented Apr 14, 2017 •

edited

Loading

adriancmiranda left a comment •

edited

Loading

adriancmiranda Apr 14, 2017 •

edited

Loading

adriancmiranda Apr 14, 2017 •

edited

Loading

adriancmiranda Apr 14, 2017 •

edited

Loading