Skip to content
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

Resolving import/compose css paths in node_module with version 2.0.0 #861

Closed
mbulfair opened this issue Dec 7, 2018 · 26 comments
Closed

Comments

@mbulfair
Copy link

mbulfair commented Dec 7, 2018

  • Operating System: OS X
  • Node Version: 10.13.0
  • NPM Version: 6.4.1
  • webpack Version: 4.27.1
  • css-loader Version: 2.0.0

Expected Behavior

Actual Behavior

Importing with css compose and import to a node module. In version 1.0.1 the ~ was not needed. In 2.0.0 it is. While in my local project I can change any references to this with no issue. However in the node_module/package/file.css if it doesn't have that same ~ then webpack/css loader can't resolve the path.

Code

Below works with 1x and partially with 2x,

also in the test of css, I am excluding a series of local node_modules, internal only packages. This may be some of the issue.

{
   loader: "css-loader",
              options: {
                modules: true,
                localIdentName: "[name]__[local]-[hash:base64:5]",
                url: false,
                importLoaders: 1
              }
            },

How Do We Reproduce?

Local File:

@value color-grey from "~@localpackage/styles/color.css";

.copyright {
  color: color-grey;
  composes: type-heading from "~@localpackage/theme/styles/typography.css";
  margin: 0;
  padding: 0;
}

Node Module

.type-heading {
  color: red
  composes: type-heading2 from "@localpackage/theme/styles/typography.css";
  margin: 0;
  padding: 0;
}

Error in resolving ☝️ with local webpack build

@alexander-akait
Copy link
Member

alexander-akait commented Dec 10, 2018

@mbulfair fixing this error introduce problems in resolving algorithm and can introduce more bugs in future (other developers can forget about using ~ for modules and it is introduce more error for next releases). i need more time to investigate how better solve this problem. As workaround we can create simple loader and add this in documentation for this package.

@alexander-akait
Copy link
Member

Why it is not easy. Let's see on:

a {
  background: url(@images/image.png);
}

Note: @ is valid symbol in any filesystem (known for me).

Now we resolve this as ./@images/image.png, but if we return resolving without ~ this can be resolving as:

  • ./@image/image.png
  • node_modules/@images/image.png

What is right resolving? Nobody knows, so we introduced ~ characters. it allows us to distinguish module requesting (from node_modules) and usual require.

In JS this problem doesn't exists because we have RFC on require where described what require('@images/image.png') mean load image.png from @image repo in node_modules. There is no such thing in css.

Why we interpreted url('image.png'), url('something/image.png') and url(@something/image.png) as relative request (i.e. ./image.png, ./something/image.png and ./@something/image.png)? For better compatibility with existing libraries and frameworks.

Feel free to feedback.

@vertic4l
Copy link

vertic4l commented Dec 18, 2018

Seems to me to be the same issue with sass-loader webpack-contrib/sass-loader#556

Somehow resolving was changed in latest release which breaks many project builds.
Maybe you could add the same fix? webpack-contrib/sass-loader@f41f791

@alexander-akait
Copy link
Member

@vertic4l it is difference issues, and doesn't exists in css-loader

Somehow resolving was changed in latest release which breaks many project builds

What currently it is break?

@vertic4l
Copy link

vertic4l commented Dec 18, 2018

@evilebottnawi well, my issue is gone. we had v0.28.x and the minimize options is gone since v1.0.
think i can remove css-loader now because sass-loader can handle the import also.

Thanks anyway! :)

Edit: can't remove it... mini-css-extract-plugin breaks without it.
Edit2: Thanks @evilebottnawi , just re-added css-loader. works as expected to me.

@alexander-akait
Copy link
Member

alexander-akait commented Dec 18, 2018

@vertic4l without css-loader your @import and urls can't work, also locals (i.e. classes and @value) doesn't work too

@alexander-akait
Copy link
Member

alexander-akait commented Dec 18, 2018

What is broken? Issue with css modules resolving as described in first comment?

@Amwam
Copy link

Amwam commented Dec 19, 2018

I'm seeing a similar issue, where the NODE_PATH isn't being respected when using compose

For instance:

my_code/css/component/myModule.css
my_code/js/component/otherModule.css

Having NODE_PATH set to my_code
Used to allow otherModule.css
to compose using

composes: thing from 'css/component/myModule.css'

instead now, only relative paths worth

composes: thing from '../../css/component/myModule.css'

@alexander-akait
Copy link
Member

@Amwam what is NODE_PATH? Don't have documentation about NODE_PATH

@Amwam

This comment has been minimized.

@alexander-akait

This comment has been minimized.

@alexander-akait
Copy link
Member

And don't spam in issue what not related to you, feel free to open new issue, comments not related to this problem will be marked as spam and will be ignored, thanks

@daniel-winter
Copy link

i need more time to investigate how better solve this problem. As workaround we can create simple loader and add this in documentation for this package.

@evilebottnawi Have you already fixed this problem? We have the same problem, when we want to update from version 1.0.0 to 2.1.0

@alexander-akait
Copy link
Member

@daniel-winter right now no, better always use ~ when you try loading something from node_modules

@iongion

This comment has been minimized.

@alexander-akait

This comment has been minimized.

@iongion

This comment has been minimized.

@alexander-akait

This comment has been minimized.

@iongion

This comment has been minimized.

@alexander-akait

This comment has been minimized.

@alexander-akait
Copy link
Member

WIP on this

@alexander-akait
Copy link
Member

/cc @daniel-winter can you provide example with bug, maybe we can solve this problem in other way.

@alexander-akait
Copy link
Member

/cc @klimashkin looks like won't fix

Why? Here answer #861 (comment)

You example:

composes: classname from 'path/to/file.css'
  • by default @import and url in css works as relative
  • if we will load path/to/file.css from modules we break other libraries

One idea to solve: we can try to resolve (https://webpack.js.org/api/loaders/#thisresolve) url/@import/from before, if module found we load this as module otherwise we load this as relative request

@alexander-akait
Copy link
Member

/cc @jquense sorry for delay, i think we need solve this before next major release 👍 Can you help me with this, thanks!

@jquense
Copy link
Contributor

jquense commented Apr 10, 2019

not sure i understand what the problem is, but it looks related to the resolution changes in v2?

@alexander-akait
Copy link
Member

Let's move discussion in #914

Please give me some feedback, we need search better solution, thanks!

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

No branches or pull requests

7 participants