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

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

Closed
wants to merge 45 commits into from

Conversation

adriancmiranda
Copy link
Contributor

@adriancmiranda adriancmiranda commented Apr 13, 2017

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.

@adriancmiranda adriancmiranda changed the title fix: useRelativePath fix: useRelativePath outputs Apr 13, 2017
@adriancmiranda adriancmiranda changed the title fix: useRelativePath outputs fix: useRelativePath Apr 13, 2017
.gitignore Outdated
!.gitignore
!.travis.yml
Copy link
Contributor

Choose a reason for hiding this comment

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

Why to change these two?

Copy link
Member

@michael-ciniawsky michael-ciniawsky Apr 14, 2017

Choose a reason for hiding this comment

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

Yep these will be taken care of by #130

Copy link
Contributor Author

@adriancmiranda adriancmiranda Apr 14, 2017

Choose a reason for hiding this comment

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

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!

index.js Outdated
@@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

The formatting change should probably be a separate PR.

Copy link
Member

Choose a reason for hiding this comment

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

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) {...}

Copy link
Contributor Author

@adriancmiranda adriancmiranda Apr 14, 2017

Choose a reason for hiding this comment

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

@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?

.gitignore Outdated
!.gitignore
!.travis.yml
Copy link
Member

@michael-ciniawsky michael-ciniawsky Apr 14, 2017

Choose a reason for hiding this comment

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

Yep these will be taken care of by #130

index.js Outdated
@@ -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
Copy link
Member

Choose a reason for hiding this comment

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

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) {...}

index.js Outdated
};
var query = loaderUtils.getOptions(this) || {};
var configKey = query.config || "fileLoader";
var options = this.options[configKey] || {};
Copy link
Member

@michael-ciniawsky michael-ciniawsky Apr 14, 2017

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 🙃

index.js Outdated
var query = loaderUtils.getOptions(this) || {};
var configKey = query.config || "fileLoader";
var options = this.options[configKey] || {};
var config = {
Copy link
Member

Choose a reason for hiding this comment

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

config => defaults ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

? 🙃

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

index.js Outdated
config[attr] = options[attr];
});
// options takes precedence over config
Object.keys(options).forEach(function(attr) {
Copy link
Member

Choose a reason for hiding this comment

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

Object.assign?

Copy link
Contributor Author

@adriancmiranda adriancmiranda Apr 14, 2017

Choose a reason for hiding this comment

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

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

Copy link
Member

@michael-ciniawsky michael-ciniawsky Apr 14, 2017

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

index.js Outdated
config[attr] = query[attr];
});
// query takes precedence over config and options
Object.keys(query).forEach(function(attr) {
Copy link
Member

Choose a reason for hiding this comment

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

Object.assign? Why is that done twice ?

Copy link
Contributor Author

@adriancmiranda adriancmiranda Apr 14, 2017

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

kk

index.js Outdated
);
}
// 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;
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

@michael-ciniawsky michael-ciniawsky Apr 14, 2017

Choose a reason for hiding this comment

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

      const issuer = {}
      const output = this.options.output || {}
      const relative = {}

      issuer.context = (this._module && this._module.issuer && this._module.issuer.context) || context;

      relative.url = issuer.ctx && path.relative(issuer.context, this.resourcePath);
      relative.path = relative.url && path.dirname(relativeUrl);

      output.dirname = relative.path
        .replace(/\.\.(\/|\\)/g, "")
        .split(path.sep)
        .join("/");

     if (output.dirname.indexOf(outputPath) !== 0) output.dirname = outputPath;

     outputPath = path.join(output.dirname, url)
          .split(path.sep)
          .join("/");
 
    
    if (output.filename && output.filename !== "extract-text-webpack-plugin-output-filename") {
       relative.path = output.dirname;
    } else if (toString.call(config.textOutputPath) === "[object String]") {
      output.context = output.path.replace(this.options.context + path.sep, "");
      output.asset = path.join(context, output.context, output.dirname);

      issuer.output = path.join(context, output.context, config.textOutputPath);

      relative.path = path.relative(issuer.output, output.asset);
    }
      url = path
       .join(relative.path, url)
       .split(path.sep)
       .join("/");
    } else if (config.outputPath) {
      url = outputPath;
    } else {
      outputPath = url;
    }

It about grouping the various values/variables together 😛

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Why ? 🙃

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@adriancmiranda adriancmiranda Apr 15, 2017

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

up-to-date

Copy link
Member

Choose a reason for hiding this comment

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

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

index.js Outdated
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, "");
Copy link
Member

Choose a reason for hiding this comment

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

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

@michael-ciniawsky
Copy link
Member

michael-ciniawsky commented Apr 14, 2017

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

Copy link
Contributor Author

@adriancmiranda adriancmiranda left a comment

Choose a reason for hiding this comment

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

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?

.gitignore Outdated
!.gitignore
!.travis.yml
Copy link
Contributor Author

@adriancmiranda adriancmiranda Apr 14, 2017

Choose a reason for hiding this comment

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

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!

index.js Outdated
};
var query = loaderUtils.getOptions(this) || {};
var configKey = query.config || "fileLoader";
var options = this.options[configKey] || {};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 🙃

index.js Outdated
var query = loaderUtils.getOptions(this) || {};
var configKey = query.config || "fileLoader";
var options = this.options[configKey] || {};
var config = {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

index.js Outdated
config[attr] = options[attr];
});
// options takes precedence over config
Object.keys(options).forEach(function(attr) {
Copy link
Contributor Author

@adriancmiranda adriancmiranda Apr 14, 2017

Choose a reason for hiding this comment

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

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

index.js Outdated
config[attr] = query[attr];
});
// query takes precedence over config and options
Object.keys(query).forEach(function(attr) {
Copy link
Contributor Author

@adriancmiranda adriancmiranda Apr 14, 2017

Choose a reason for hiding this comment

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

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

index.js Outdated
);
}
// 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;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@michael-ciniawsky
Copy link
Member

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
Copy link
Contributor Author

adriancmiranda commented Apr 15, 2017

@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.

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

Choose a reason for hiding this comment

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

👍

Copy link
Contributor Author

@adriancmiranda adriancmiranda Apr 15, 2017

Choose a reason for hiding this comment

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

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
Copy link
Member

@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
Copy link
Contributor Author

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

@codecov
Copy link

codecov bot commented Jun 8, 2017

Codecov Report

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

Impacted file tree graph

@@            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
Copy link

stereokai commented Oct 16, 2017

@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
Copy link
Contributor Author

adriancmiranda commented Oct 16, 2017

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
Copy link

@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
Copy link
Contributor Author

adriancmiranda commented Oct 25, 2017

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

@stereokai
Copy link

@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
Copy link
Contributor Author

adriancmiranda commented Nov 27, 2017

@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
Copy link

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

@adriancmiranda
Copy link
Contributor Author

adriancmiranda commented Nov 27, 2017

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

@stereokai
Copy link

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

@adriancmiranda
Copy link
Contributor Author

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

@Leward
Copy link

Leward commented Mar 8, 2018

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
Copy link
Contributor Author

adriancmiranda commented Mar 8, 2018

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

@michael-ciniawsky michael-ciniawsky changed the title feat: add option.cssOutputPath feat(index): add cssOutputPath option (option.cssOutputPath) Aug 21, 2018
Copy link
Member

@michael-ciniawsky michael-ciniawsky left a comment

Choose a reason for hiding this comment

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

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants