Leaflet / Leaflet

🍃 JavaScript library for mobile-friendly interactive maps 🇺🇦
https://leafletjs.com
BSD 2-Clause "Simplified" License
40.28k stars 5.76k forks source link

leaflet.css and webpack css-loader #4849

Closed jmlopez-rod closed 7 years ago

jmlopez-rod commented 7 years ago

In the commit https://github.com/Leaflet/Leaflet/commit/e9957cfa02b4201c62e98d43acc23acd5f3f9927 there is the following change:

+.leaflet-default-icon-path {
+   background-image: url(images/);
+   }

But now, in webpack with the css-loader I get the following error:

ERROR in ./~/css-loader!./~/leaflet/dist/leaflet.css
Module not found: Error: Cannot resolve directory './images' in /Users/jmlopez/Workspace/ProjectName/node_modules/leaflet/dist
 @ ./~/css-loader!./~/leaflet/dist/leaflet.css 6:8715-8735

If I make it a proper url to a file then the compilation goes through. Anyone else having this issue? Should I bring this up with https://github.com/webpack/css-loader?

IvanSanchez commented 7 years ago

Yeah, the method of detecting the image path in #4605 involves using a background image which is just a path, and doesn't exist.

It's a bit counter-intuitive, since we're using that just for path resolution and not actually for setting the background. Browsers don't even try to load the non-existent image, as there only element with that CSS class is destroyed before being used.

It's a fringe case, but we won¡t change any code when the incompatibility is due to a third party (it's not that we have something against webpack, but we ignore bugs related to R, Vue, React, Angular, Ionic, and so on and so on).

Maybe it's a good idea to make this a warning in Webpack instead of an error.

soyjavi commented 7 years ago

IMHO, is better a warning... a lot of people uses 3rds like leaflet

nikolauskrismer commented 7 years ago

Is there a workaround (maybe within webpack or by using arguments in argument given to require) that anybody knows of? This prevents me from using leaflet 1.0.0 (since I use webpack)

IvanSanchez commented 7 years ago

@nikolauskrismer Ask around the webpack folks and community. We'll be happy to apply a pull request to make webpack work nicer with Leaflet 1 if it doesn't break the functionality of marker icon path autodetection. None of the Leaflet team currently makes heavy use of webpack.

paralin commented 7 years ago

@IvanSanchez I would re-open this. There are a LOT of people using webpack. I've read through that entire PR explaining why using a background-image to a non-existent directory path is OK, but it still doesn't make sense at all to me. Lots of browsers will actually make the request to images/. Supposing the server has a 404 policy that returns index.html, or that we track 404s or 403s. I don't like the idea at all that my server's going to be bouncing tons of network requests for absolutely no reason.

Just seems like a poor hack that could be avoided in some other way.

IvanSanchez commented 7 years ago

Lots of browsers will actually make the request to images/.

No, they don't. We actually tested the technique in the major 4 browser engines, and none of them made any requests, as the DOM elements are removed before they request the network resources.

Just seems like a poor hack that could be avoided in some other way.

It's way better than the downsides of the previous, uglier hack, believe me.


I'll reopen this, but I want to make myself clear: None of the active people in the Leaflet team use webpack, so I hope an experienced webpack user will suggest an approach to this problem.

paralin commented 7 years ago

@IvanSanchez I understand it's a good solution to the problem you had before, I think there's a fundamental issue here in how asset bundlers like Webpack will treat leaflet - they will analyze the css and copy the images into the output bundle in a specific way, and then replace the url reference in the css with something else. They expect the url() definitions to actually point to files rather than act as markers for other more complex loading down the line.

I think the solution is to, specifically for WebPack, disable the url evaluation in css-loader (this is already an option), and then to tell Webpack to copy the images/ directory into the output bundle as-is without modification.

jasongrout commented 7 years ago

I just ran into this issue with webpack too, and spent some time before we realized how leaflet is abusing the url() attribute here to have a setting that you can override. I think an easy thing to do is to change the url to point to an actual file (which was mentioned on the original PR here). A disadvantage would be that it would request a file from the server when checking the url, but it would be much more in line with the CSS spec and wouldn't trip up things that assume that url() references are valid resources.

grantHarris commented 7 years ago

I echo the previous comments. My build is broken and I haven't found a workaround yet.

jmlopez-rod commented 7 years ago

@jasongrout According to @IvanSanchez the file would not be requested. So I agree with your solution. Make it point to a dummy file, just call it dummy.png or whatever as long as the css-loader can find it. In the end leaflet doesn't use it. It's either that or someone in the css-loader can make it a warning.

jefbarn commented 7 years ago

Yeah, you're already loading 'images/layers.png' in leaflet-control-layers-toggle, just use that CSS class to find the path and strip off the file name.

jasongrout commented 7 years ago

Replying to several comments above...

It's a fringe case, but we won¡t change any code when the incompatibility is due to a third party (it's not that we have something against webpack, but we ignore bugs related to R, Vue, React, Angular, Ionic, and so on and so on).

@IvanSanchez - this is about adhering to the CSS spec and intent and not abusing it (too much). The current implementation of having an invalid url resource relies on implementation details of browsers to not have errors. Webpack's css-loader is just trying to follow the CSS spec here and retrieve the url. I think Webpack's css-loader has a much better case to say that leaflet is the one that is incompatible with the CSS spec (if not with the letter, then certainly with the intent) and should change.

Make it point to a dummy file, just call it dummy.png or whatever as long as the css-loader can find it.

The comment in the original PR proposed images/marker-icon.png, which does exist.

Yeah, you're already loading 'images/layers.png' in leaflet-control-layers-toggle, just use that CSS class to find the path and strip off the file name.

I don't think that serves the goal of having a path that is easy to override.

jasongrout commented 7 years ago

Would you be open to a PR that implements the suggestion from the other PR to change the url to a valid resource? I'm happy to make such a PR.

IvanSanchez commented 7 years ago

@jasongrout Absolutely yes, pull reqs are welcome! :+1:

(Just make sure to read the contributing guide, specifically the part about running the unit tests, to make sure that L.Icon.Default behaves the same after your changes)

mourner commented 7 years ago

Adding this to 1.0.1 milestone.

Eschon commented 7 years ago

I just ran into this problem.

My workaround for now is to copy the css and the images to my folder with other assets that aren't processed by webpack and add a link tag for the css in my html template.

jasongrout commented 7 years ago

I have a patch with a test that I'll hopefully be able to submit soon, as soon as I get permission to contribute to Leaflet.

IvanSanchez commented 7 years ago

@jasongrout Don't wait for permission, fork the repo and submit a pull request with your changes instead.

jasongrout commented 7 years ago

I'm waiting for permission from my employer, not you :).

IvanSanchez commented 7 years ago

@jasongrout OK, you gotta admit your choice of words was ambiguous :-D

jasongrout commented 7 years ago

That commit referenced above by @zv3 looks better than my solution - seems like a great idea to use a data URL!

IvanSanchez commented 7 years ago

No, @zv3's hack makes the path detection fail.

zv3 commented 7 years ago

Yeah, my bad, I deleted it for that reason, tests passed ok but wasn't sure it would actually work! Not sure why the commit references are still here 😐

jefbarn commented 7 years ago

How about something like:

/* Default icon URLs */
.leaflet-default-icon-path {
    content: 'images/';
    }
paralin commented 7 years ago

I like @jefbarn solution a lot better.

IvanSanchez commented 7 years ago

@jefbarn Nope. That will not resolve to the full path of the CSS file.

mourner commented 7 years ago

https://github.com/Leaflet/Leaflet/pull/4979 should have fixed this. Can you all please make sure the master branch works fine for you in Webpack?

Eschon commented 7 years ago

Just tried it and it works for me. Thanks!

MatthewEdge commented 7 years ago

Sorry to post in a closed topic but just in case others Google the same issue:

When using Webpack 2.3.1 and Leaflet 1.0.3 and the following config:

{
    test: /\.css$/,
    use: [
        { loader: 'style-loader' },
        {
            loader: 'css-loader',
            options: {
                sourceMap: true,
                **modules: true,**
                localIdentName: '[name]__[local]--[hash:base64:5]'
            }
        }
    ]
}

Webpack reports the issue stated above during bundling. If, however, we remove the modules: true part of the css-loader config the build works as expected.

Not sure if others see the same thing but just wanted to post our findings

tthew commented 6 years ago

For anyone else landing here and struggling with this, the following webpack2 resolve.alias configuration should enable you to import the leaflet.css regardless of whether you're using CSS modules or not:

  resolve: {
    // Leaflet image Alias resolutions
    alias: {
      './images/layers.png$': path.resolve(__dirname, '../node_modules/leaflet/dist/images/layers.png'),
      './images/layers-2x.png$': path.resolve(__dirname, '../node_modules/leaflet/dist/images/layers-2x.png'),
      './images/marker-icon.png$': path.resolve(__dirname, '../node_modules/leaflet/dist/images/marker-icon.png'),
      './images/marker-icon-2x.png$': path.resolve(__dirname, '../node_modules/leaflet/dist/images/marker-icon-2x.png'),
      './images/marker-shadow.png$': path.resolve(__dirname, '../node_modules/leaflet/dist/images/marker-shadow.png')
    }
  }

_n.b the actual path to the images may differ depending on where your webpack config file is in relation to nodemodules

tomcgn commented 6 years ago

The resolve.alias did not work in my case, also on webpack. Instead, these two steps solved the marker icon issue:

  1. In webpack.common.js add or extend a CopyWebPackPlugin section:
    new CopyWebpackPlugin([
              ...
                { from: './src/main/webapp/favicon.ico', to: 'favicon.ico' },
                { from: './src/main/webapp/marker-icon.png', to: 'marker-icon.png' },
                { from: './src/main/webapp/marker-shadow.png', to: 'marker-shadow.png' },
                ...
                { from: './src/main/webapp/robots.txt', to: 'robots.txt' }
            ])
  2. place the .png files into src/webapp
  3. do a full build
foodaka commented 6 years ago

im also having issues using leaflet with webpack 2 or 3. Is there any solution to this besides @tthew or @tomcgn ? @MatthewEdge what was the workaround for this?

altaudio commented 6 years ago

The way we got round this was to use a postinstall script in package.json to modify the image locations to be absolute rather than relative using the NPM replace-in-file package:

// makeReferencesAbsolute.js

const replace = require('replace-in-file')

const options = {
  files: 'node_modules/leaflet/dist/leaflet.css',
  from: /images/g,
  to: '~leaflet/dist/images'
}

replace(options)
  .then(changes => {
    console.log('Modified files:', changes.join(', '))
  })
  .catch(error => {
    console.error('Error occurred:', error)
  })

Your reference to node_modules may differ based on your folder structure.

Then, in package.json:

// package.json

"scripts": {
    ...
    "postinstall": "node makeReferencesAbsolute.js"
  },

The leaflet CSS file is then modified so the image locations are absolute rather than relative on package install.

juiceboxjoe commented 6 years ago

@MatthewEdge your solution worked for me as well.

faelplg commented 6 years ago

I think the problem that brought me here is the same. Here is my solution without the need of a workaround.

Obs.: When I started having these errors, I was using html-loader and css-loader, among others for js and json.

Running leaflet with webpack was throwing an error about the .PNG images addresses from leaflet.css. I understood that It was trying to load the images as a CSS. So, the main question was how to provide a file loader in webpack that can read the .PNG files.

There is a package (file-loader) that solved this in a simple way. Below is how I used it:

Packages:

"leaflet": "^1.3.1",
"webpack": "^2.7.0".
"file-loader": "^1.1.11"

In weback.conf.js file, I included this code:

module: {
    loaders: [
      // ...the other loaders for html, css and js
      {
        test: /\.(gif|svg|jpg|png)$/,
        loader: 'file-loader',
      }
    ]
  } // ...the plugins and etc
panuhorsmalahti commented 5 years ago

If using webpack 4 with css modules I got this working with the following.

Importing the file:

import "leaflet/dist/leaflet.css";

To handle loading the images:

     resolve: {
        alias: {
            "./images/layers.png$": path.resolve(
                __dirname,
                "./node_modules/leaflet/dist/images/layers.png"
            ),
            "./images/layers-2x.png$": path.resolve(
                __dirname,
                "./node_modules/leaflet/dist/images/layers-2x.png"
            ),
            "./images/marker-icon.png$": path.resolve(
                __dirname,
                "./node_modules/leaflet/dist/images/marker-icon.png"
            ),
            "./images/marker-icon-2x.png$": path.resolve(
                __dirname,
                "./node_modules/leaflet/dist/images/marker-icon-2x.png"
            ),
            "./images/marker-shadow.png$": path.resolve(
                __dirname,
                "./node_modules/leaflet/dist/images/marker-shadow.png"
            )
        }

      module: {
            {
                test: /\.(gif|svg|jpg|png)$/,
                loader: "file-loader"
            }

To work with CSS modules (e.g. exclude CSS modules for leaflet):

            {
                test: /\leaflet.css$/,
                use: [
                    {loader: "style-loader"},
                    {loader: "css-loader"}
                ]
            },
            {
                test: /\.css$/,
                exclude: /\leaflet.css$/,
                use: [
                    {loader: "style-loader"},
                    {
                        loader: "css-loader",
                        options: {
                            modules: true
                        }
                    }
                ]
            },
alessandro-fazzi commented 5 years ago

I'd like to add as a note for Webpack users: if you webpack config will add hash to images, the replace in Icon.Default.js:55 will blow up

path = path.replace(/^url\(["']?/, '').replace(/marker-icon\.png["']?\)$/, '');

because your image name at that point will be something like marker-icon-fd765dfabc876987.png.

ghybs commented 5 years ago

Hi @pioneerskies,

For reference, there is now a leaflet-defaulticon-compatibility plugin to make webpack + style-loader + css-loader + file-loader/url-loader happy without further configuration.

See https://github.com/Leaflet/Leaflet/issues/4968#issuecomment-399857656

jazdw commented 4 years ago

@pioneerskies

I'd like to add as a note for Webpack users: if you webpack config will add hash to images, the replace in Icon.Default.js:55 will blow up

Yep the problem still exists in 1.5.1, in my case the hash is a query paramer and _detectIconPath() ends up thinking the path is something like /web/img/marker-icon.png?v=2273e3d8ad9264b7daa5bdbf8e6b47f8")" The RegExp should really be fixed or use proper URL parsing (doesn't work in IE).

As a work around you can set L.Icon.Default.imagePath manually.

bart-le commented 3 years ago

Packages:

This one (#6951) might be more up to date. Adding options to the file-loader rules worked for me. If you're running Webpack in developer mode, the workaround should do it:

/* webpack.dev.js */

module.exports = {
  mode: "development",
  /* your paths and other configs */
  module: {
    rules: [
      /* loaders */
      {
        test: /\.(png|jpe?g|gif)$/i,
        use: [
          {
            loader: "file-loader",
            options: {
              name: "[path]/[name].[ext]",
            },
          },
        ],
      },
    ],
  },
};

Then I imported it my .scss file. import "leaflet/dist/leaflet.css"; in your .js file will work as well if you prefer it.

/* _map.scss */

@import "~leaflet/dist/leaflet.css";

.leaflet-container {
    width: 100%;
    height: 100vh;
}
Martskin commented 11 months ago

I would suggest just using Base64 for the images in the stylesheet to avoid this issue altogether.