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

feat: Emit files with relative urls #135

Merged
merged 20 commits into from
Mar 31, 2017
Merged

feat: Emit files with relative urls #135

merged 20 commits into from
Mar 31, 2017

Conversation

adriancmiranda
Copy link
Contributor

@adriancmiranda adriancmiranda commented Mar 9, 2017

What kind of change does this PR introduce?
A feat

Did you add tests for your changes?
The old tests should work.

If relevant, did you update the README?
The old docs should work too.

Summary

Emit files with relative urls
Fixes #62 and #46

Does this PR introduce a breaking change?

no

@jsf-clabot
Copy link

jsf-clabot commented Mar 9, 2017

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@joshwiens joshwiens left a comment

Choose a reason for hiding this comment

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

The new configuration option needs to be documented in the readme.

Needs tests specific to the above configuration object.

@adriancmiranda
Copy link
Contributor Author

adriancmiranda commented Mar 9, 2017

@d3viant0ne I'm sorry for the sequence of 'commit'. I'll do it all soon. Thank´s!

@adriancmiranda
Copy link
Contributor Author

Done!

@joshwiens joshwiens requested a review from bebraw March 9, 2017 21:04
Copy link
Contributor

@bebraw bebraw 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 to me.

@huan086
Copy link

huan086 commented Mar 28, 2017

Doesn't seem to handle my case

module.exports = {
    entry: { "js/server": [ "./folder1/script1.jsx", "./folder2/script1.jsx" ] }
...

./folder1/script1.jsx has require("../content/images/picture.png") (relative to folder1)

The generated path is

module.exports = __webpack_require__.p + "../content/images/content/images/picture.png";

when I expected it to be

module.exports = __webpack_require__.p + "../content/images/picture.png"; (relative to js)

I'm running Windows

Option used is

{
        test: /\.(png|gif|woff2?|eot|ttf|svg)$/,
        use: [
            {
                loader: "url-loader",
                options: {
                    limit: 10000,
                    emitFile: true,
                    name: "[path][name].[ext]",
                    useRelativePath: true
                }
            }]
    }

@adriancmiranda
Copy link
Contributor Author

adriancmiranda commented Mar 28, 2017

Sorry for close, I was driving when I receive the note and I made a mess...

@huan086 Remove the [path] from name and try again.

@huan086
Copy link

huan086 commented Mar 29, 2017

@adriancmiranda nice catch!

There's another problem, which I don't know if it should be fixed by url-loader or file-loader or ExtractTextPlugin or webpack itself

My output folder is

    output: {
        path: path.resolve(__dirname, ".")

In ./folder1/folder2/script1.jsx, i have an import "../../content/bootstrap.css";

In bootstrap.css, it has

@font-face {
  src: url('../fonts/glyphicons-halflings-regular.eot');

Folder structure

root
|-content
| |-bootstrap.css
| `-fonts
|   `-glyphicons-halflings-regular.eot
`-folder1
  `-folder2
    `-script1.jsx

It resulted in server.css

src:url(../fonts/glyphicons-halflings-regular.eot)

which is invalid and ./fonts/glyphicons-halflings-regular.eot being emitted

Expected output
Folder structure

root
|-content
| |-bootstrap.css
| `-fonts
|   `-glyphicons-halflings-regular.eot
|-folder1
| `-folder2
|   `-script1.jsx
`-js
  |-server.js
  `-server.css

I think for the issue 46 involving CSS, it can never be fixed as long as webpack don't handle CSS separately
#46 (comment)

@joshwiens joshwiens changed the title Emit files with relative urls fix: Emit files with relative urls Mar 29, 2017
@adriancmiranda
Copy link
Contributor Author

adriancmiranda commented Mar 29, 2017

@huan086 I don't know if I understand the problem(Mainly without view the webpack.config), you mean something like this?
https://bitbucket.org/adriancmiranda/pr-file-loader/src

@huan086
Copy link

huan086 commented Mar 29, 2017

Wow! Close enough. In https://bitbucket.org/adriancmiranda/pr-file-loader/src/266ff6b5bd76b4f434cf21a61904f1198e36e1e5/content/bootstrap.css?at=master&fileviewer=file-view-default

change to url("../fonts/bold-cyrillic-ext.woff2") (double dots)

I'm thinking of using the following to fix

  1. Change all URL in css to absolute path relative to root. e.g. /content/fonts/bold-cyrillic-ext.woff2 in either postcss-loader or css-loader
  2. When it reaches url-loader, treat absolute path as relative to root, and modify the path to become relative to the javascript file containing the module.export. e.g. <output directory>/server.js would have module.export = "@font-face { ... url(./content/fonts/bold-cyrillic-ext.woff2) ... }
  3. file-loader would emit files under <output directory>/content/fonts/bold-cyrillic-ext.woff2
  4. ExtractTextPlugin, when configured to output the css to a directory other than <output directory>/server.css, would modify the url accordingly.

I'm new to webpack, no idea whether the above would work or not...

Edit
What was I thinking?! All that's needed is to have a PostCSS plugin to convert all url to absolute i.e. /content/fonts/bold-cyrillic-ext.woff2 and the CSS output will be correct. No need to change url-loader and ExtractTextPlugin.

Only need to verify that file-loader is able to emit to the correct folder in output directory

@adriancmiranda
Copy link
Contributor Author

adriancmiranda commented Mar 29, 2017

Ok, let me see:

If you put ../fonts (with double dots), the require method can't resolve this path because doesn't exists fonts directory on the root directory, this will generate an error, anyway, if I put the fonts directory in the root, generates an outputPath correct, but with a unexpected url in this specific case (pointing to a directory in the root).

expected:
fonts

obtained:
../fonts (without any change because the issuer context is pointing to up in relation to the font)

I'll fix this also validating the webpack public path parameter, not only outputPath (As I already do). Something like path.relative(context, outputPath) and they should be working, I think I can see about this later.

About the other things i.e. PostCSS and absolute paths

It solve but isn't the proposed solution. I think the file-loader is perfectly able to solving this, because he has access to all needed urls and you can still use context, publicPath and outputPath to help you.

@joshwiens joshwiens changed the title fix: Emit files with relative urls feat: Emit files with relative urls Mar 30, 2017
@joshwiens
Copy link
Member

@adriancmiranda - Do you have anything else needing to be updated here pre you last comment or is this good to go?

@adriancmiranda
Copy link
Contributor Author

adriancmiranda commented Mar 30, 2017

@d3viant0ne I prefer keep it simple for now, so yes, it's good to go!

@joshwiens
Copy link
Member

@bebraw @michael-ciniawsky - Anything else before we we land this feature?

@bebraw
Copy link
Contributor

bebraw commented Mar 31, 2017

@d3viant0ne Nope. It's good to go.

@adriancmiranda
Copy link
Contributor Author

adriancmiranda commented Apr 12, 2017

@huan086 I updated the sample folder, may you test for me again, please?
https://bitbucket.org/adriancmiranda/pr-file-loader/src
It should be working for your case too.

@Jorybraun
Copy link

I am having the same issue,
about the output paths in the parent folder.

@adriancmiranda
Copy link
Contributor Author

@Jorybraun This was already been solved, follow it here, please 😉

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

6 participants