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

Optional ~ for modules #74

Closed
donaldpipowitch opened this issue Jan 28, 2016 · 27 comments
Closed

Optional ~ for modules #74

donaldpipowitch opened this issue Jan 28, 2016 · 27 comments

Comments

@donaldpipowitch
Copy link

Why do I need to prepend ~ for modules? I'd rather prepend ./ for relative files. This seems to work great, when I comment this line: var moduleRequest = loaderUtils.urlToRequest(filename, query.root);

@import "module-foo/style.less"; // loads from my modules
@import "./module-bar.less"; // loads relative file

Can we make ~ optional?

@donaldpipowitch
Copy link
Author

My problem could actually be solved by supporting Less paths option implemented in this pull request: #75.

@jhnns
Copy link
Member

jhnns commented Mar 24, 2016

I would like to change that as well... it's counter-intuitive and forces you to write your less files in a way that just webpack is able to understand it.

However, it was @sokra 's decision back then and I don't know if it's wise to change that now with a major version bump. That would probably force a lot of people to refactor their code 😁

@donaldpipowitch
Copy link
Author

It would be nice to see this change though, so our Less modules can be loaded with a non-webpack-stack as well.

@jhnns
Copy link
Member

jhnns commented Mar 25, 2016

I've thought about this today. We could make it optional by first doing a lookup with urlToRequest and then without urlToRequest. Thus we won't break existing code and enable the lookup as "fallback".

However, the problem still remains with url() imports of images, because these are handled by the css-loader. So, the goal "compilation with a non-webpack-stack" is only possible as long as you don't need to reference images in other packages. That's why I would like to have @sokra 's opinion about it if we could deprecate the ~ import.

It's a tough decision because the ~ makes sense if you want to preserve CSS' native way of referencing other resources 😞. However, with modern build tools, people get used to 1. somehow precompile their stuff and 2. distinguish between relative and package (aka node_modules) imports. So this is probably a valid change.

Additionally, we should align this decision with the sass-loader, as it is very similar to the less-loader.

@jhnns
Copy link
Member

jhnns commented Mar 25, 2016

Concerning your PR #75: I'd first like to have a decision here. Because in case we remove ~ imports, I'd rather not implement the includePath option because there should be only one way to handle your imports.

@donaldpipowitch
Copy link
Author

That's fine. I totally understand your concerns and didn't knew that css-loader is responsible for image loading. You're right: this should be addressed in all loaders. For all loaders I'd like to either use relative (./foo) or absolute (/foo) paths and for everything else (foo) try loading via node resolve logic.

@sokra
Copy link
Member

sokra commented Mar 26, 2016

CSS and less only know about relative urls, they have no idea what "modules" are... Both less-loader and css-loader are designed to handle existing code well. So url(file) and @import "file" resolve relative by default. Existing libraries are written this way. (i. e. https://github.com/twbs/bootstrap/blob/master/less/bootstrap.less)

So the ~ is a webpack-only escape char to point to modules. It leaves the existing behavior intact and only adds ~ as feature.

The less-loader aligns with the underlying less language. It doesn't change the language it only adds the ~ feature.

So the best way to add modules to the less-loader would be to add module urls to the less language. Maybe something like @import (module) "module-foo/file". This would be compatible to exisiting code.

Trying to resolve both, first normal, than as module, would be a webpack-only extension to the less language and would not be better than ~ (which is also webpack-only). Both make you write code which is webpack-only... :(

Related:

@jhnns
Copy link
Member

jhnns commented Mar 27, 2016

Existing libraries are written this way.

Yes, correct. That's why we should not change the semantics. Basically, writing import "foo" should always look at ./foo first – or otherwise we would be incompatible with a lot of libraries.

Trying to resolve both, first normal, than as module, would be a webpack-only extension to the less language and would not be better than ~

I don't think that this would be a webpack-only way. It's basically the same as when using the --include-path option. Unfortunately, this only works for LESS imports, not for url() statements, which are not resolved by LESS.

@donaldpipowitch
Copy link
Author

donaldpipowitch commented Mar 27, 2016

So url(file) and @import "file" resolve relative by default. Existing libraries are written this way. (i. e. https://github.com/twbs/bootstrap/blob/master/less/bootstrap.less)

Yes, but I'd expect that the bower Bootstrap maintainers would maybe accept a pull request to change an import from @import "file" to @import "./file" for explicit relative loading, but I doubt they would add a ~ (if they would load a dependency themselves), if that would mean all their users would have to use webpack.

@jhnns
Copy link
Member

jhnns commented Mar 31, 2016

With "bower" you mean LESS? 😀

And yes, LESS is actually considering to enable this via option

@donaldpipowitch
Copy link
Author

donaldpipowitch commented Mar 31, 2016

Sorry, I meant "Bootstrap" instead of "Bower", because the example sokra mentioned was Bootstrap :D

@donaldpipowitch
Copy link
Author

I'd like to come to a conclusion here. As seen in #79 and #81 more people are bitten by this recently. I'll try to breakdown the current status quo:

  • Less resolves files relative by default.
  • Less has no built-in module resolving logic, but you can point to module directories like node_modules or bower_components with the paths. (It is also possible to use plugins like this one.)
  • PR change context by using paths array #75 adds paths to less-loader, but as @jhnns said, this only works for importing Less files, not url() statements.
  • For now less-loader required us to prefix file paths with ~ to use Webpacks module resolving logic. This requires every developer to use Webpack, if the developer wants to use a Less file using this.

My proposal: Try resolving Less files relative by default. If you can't find a relative file use Webpacks module resolving logic to find a corresponding module (just like it would work if you use paths and no Webpack). The same logic will be used for url() statements and other referenced files. No need for ~ and no need for PR #75. If you want to use relative files explicitly you can use ./.

@ritz078
Copy link

ritz078 commented Apr 18, 2016

It will be really nice if we can write code that isn't only for webpack. Currently, the compulsion to use ~ is the reason I can't integrate webpack in my project .

@timmfin
Copy link

timmfin commented Apr 20, 2016

Surprised no one has yet mentioned omit-tilde-webpack-plugin. While it does not directly solve the problem, it is very related and may help a few folks out (or maybe show some precedent for optional ~... at least for hacking in support for legacy/non-webpack-aware code?).

@jeffijoe
Copy link

I agree, is there some technical reason why we can't do this?

@donaldpipowitch
Copy link
Author

@timmfin Wow, thank you. I have to try that with our code base!

@jhnns
Copy link
Member

jhnns commented Apr 22, 2016

I agree, is there some technical reason why we can't do this?

I don't think so. Imho it should be safe to prefer the local one first and then to look for module directories. But for a consistent behavior this needs to be adjusted in the less-loader, sass-loader, css-loader and html-loader (and probably some more, but these are the most important ones imho). That's why I would like to have @sokra 's commitment...

@AndyOGo
Copy link

AndyOGo commented May 19, 2016

Guys please keep stuff sane and stick to standards, if less supports a --inclue-path option, less-loader for webpack should do so too.
It's really not worth having a discussion lasting for months about an obvious fault.

@jhnns
Copy link
Member

jhnns commented May 19, 2016

It's really not worth having a discussion lasting for months about an obvious fault.

It's not an obvious fault. It's a valid discussion. And as I've already pointed out: it needs to be consistent across different loaders (e.g. the css-loader resolves url() statements).

@AndyOGo
Copy link

AndyOGo commented May 19, 2016

@jhnns
Thank you for your very quick answer.
I really think we have to separate concerns here.
I want to point out that less's intrinsic features should stay intact and should not be broken by any tool around it.
How less deals with @import should be completely unrelated to url() statements processed by a following module. That should be strictly disconnected. You gain portability and simplicity if you follow this policy.

@donaldpipowitch
Copy link
Author

donaldpipowitch commented May 31, 2016

FYI: The omit-tilde-webpack-plugin doesn't seem to work for dependencies of dependencies. bholloway/omit-tilde-webpack-plugin#5

@bholloway
Copy link

My intention in writing omit-tilde-webpack-plugin was to aid to migration. Personally I think disambiguation is needed and that is why I moved all my code to the tilde ~ syntax.

However who is to say that @donaldpipowitch's use case is not just a ...really...long...migration... (wink)

Super busy right now but will try to take a look at bholloway/omit-tilde-webpack-plugin#5.

@donaldpipowitch
Copy link
Author

donaldpipowitch commented May 31, 2016

However who is to say that @donaldpipowitch's use case is not just a ...really...long...migration... (wink)

I really just want to re-use our existing Less modules (about ~20 or so?) which should continue to work with the vanilla Less compiler without webpack. I don't want to write a lot more modules in this style. (Ideally we would switch to something completely different than Less in the future anyway :))

Thank you for investigating.

But besides migration... I still wouldn't want to publish new projects in Less, Sass or whatever with ~ if this forces other developers to use webpack to consume my modules :/

@jhnns
Copy link
Member

jhnns commented Jun 1, 2016

vanilla Less compiler without webpack

I can totally understand this desire. However, I think it would also be sufficient to have a tilde-less-plugin (similar to the less-plugin-npm-import) which allows you to compile LESS modules without webpack.

@donaldpipowitch
Copy link
Author

I think it would also be sufficient to have a tilde-less-plugin

Yeah. Actually that is quite a nice idea. I already developed Mercateo/less-plugin-bower-resolve before. That was 2 years ago... I didn't even thought about something like that... Don't know how it would work with url() and other corner cases... But could be solvable...

@donaldpipowitch
Copy link
Author

FYI: My issue with omit-tilde-webpack-plugin was resolved now. When I use this plugin webpack basically behaves the way I need it. I'd still use this plugin outside of migration use cases, so I don't force other developers to use webpack when they use my components.

Thanks everyone for participating in this discussion. I leave it up to you to close this issue or to discuss further, if this should be the default behaviour of webpack.

@jhnns
Copy link
Member

jhnns commented Mar 20, 2017

Starting with less-loader 4, you can now choose between webpack's resolver and the default Less resolver: https://github.com/webpack-contrib/less-loader#imports

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

No branches or pull requests

8 participants