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

Support for tilde character to import from node_modules #2350

Closed
yogeshgadge opened this issue Jul 21, 2017 · 15 comments
Closed

Support for tilde character to import from node_modules #2350

yogeshgadge opened this issue Jul 21, 2017 · 15 comments

Comments

@yogeshgadge
Copy link

Proposal

Add support for tilde character to import a module from node_modules.
Current workaround of 'includePath` makes the build inefficient when we have

e.g. in my styles.scss I can do the following
@import '~bootstrap-sass/assets/stylesheets/bootstrap/alerts';
instead of
@import 'bootstrap-sass/assets/stylesheets/bootstrap/alerts';
and somewhere
incudePaths: ['./node_modules'']

My guess is that this inefficiency increases when we have many npm modules as dependencies since the search operation has to search inside the entire node_modules. Trying to improve some efficiency to narrow includePaths is very fragile because the libraries we import may import other libraries and can't be really done without trial and error or having a full knowledge of all the artifacts and their nested imports which defeats the purpose of having libraries in the first place.

Side note: Some build systems have added this feature e.g. https://github.com/webpack-contrib/sass-loader. However those who do not use webpack are at loss.

@ArmorDarks
Copy link

I'm not sure, though, how this helps with performance concerns.

@yogeshgadge
Copy link
Author

It is like searching a path/fragment inside node_modules vs saying the path is a definitive path /node_module/path/fragment i.e. no more searching. The tilde character makes that definitive by using ~path/fragment.

A bit of a background...
I have a webpack project where I distribute my SASS files which rely on other node_modules. Fortunately webpack has a sass-loader plugin that has the same concept as the tilde character however when my module is used by other projects that does not use sass-loader plugin (part of webpack).....say they use rollup for build instead of webpack my only option is to specify path/fragment and have includePaths:['node_module']. As as soon as I started doing this I have seen some slowness in my build.
With this being the only change I began to wonder the cause of it which lead to none other than the above hypothesis.

@ArmorDarks
Copy link

Well, I have some concerns regarding ~ character. It doesn't sound like a convention, besides some people might be confused by such close attaching to Node environment. For example, there is a Ruby, where people will also need to import from gems, and here ~ semantic really becomes obscure.

And it seems to me that package management is something, that should be handled not by Sass, since Sass just a compiler, nothing more.

Btw, for most cases it just enough to import directly from node_modules and do not use incudePaths:

@import 'node_modules/ekzo/objects/breadcrumb';

As a benefit, it makes clear where exactly that stylesheet coming from.

It will work for most cases, unless import has its own dependencies, but issue with dependencies is far beyond scope of this issue.

@nex3
Copy link
Contributor

nex3 commented Jul 26, 2017

As a rule, special resolution of imports are the domain of importer plugins, not the implementation itself. It's possible that this makes sense for a wrapper, such as node-sass, but not for the core language.

@penx
Copy link

penx commented Apr 11, 2018

When building a Sass library that will be distributed via npm, that depends on other Sass libraries that are distributed on npm, what syntax should be used for the import?

@import 'node_modules/ekzo/objects/breadcrumb'; will cause issues with monorepos that hoist dependencies up.

Official language support for @import '~ekzo/objects/breadcrumb'; seems like the best solution to me.

I think this question is independent of node-sass as it's not assuming node-sass would be used for the build, just that npm will be used for the distribution.

@ArmorDarks
Copy link

@penx I'd recommend taking a look at Eyeglass.

@nex3
Copy link
Contributor

nex3 commented Apr 12, 2018

Eyeglass is a good solution for this. You can also just write your imports like ekzo/objects/breadcrumb and use the includePaths option to ensure that your node_modules directory is on the include path.

@ArmorDarks
Copy link

You can also just write your imports like ekzo/objects/breadcrumb and use the includePaths option to ensure that your node_modules directory is on the include path.

As a side note, I'd probably recommend using 'node_modules/ekzo/objects/breadcrumb' path instead of includePaths option, since it makes clearer where that dependency coming from. Though, it's just imho.

@yogeshgadge
Copy link
Author

yogeshgadge commented Apr 13, 2018 via email

@penx
Copy link

penx commented Apr 13, 2018

As a side note, I'd probably recommend using 'node_modules/ekzo/objects/breadcrumb' path instead of includePaths option

But this is problematic if your peer project is hoisting dependencies

@penx
Copy link

penx commented Apr 13, 2018

At the moment, Sass library creators have a choice of:

  • using ~ but requiring the peer project to use sass-loader or something else that supports it
  • using node_modules but not supporting peer projects that hoist dependencies
  • using Eyeglass which I think would stipulate that the peer project must also use/setup Eyeglass

At the moment, using ~ seems the most convenient approach, Eyeglass seems the most reliable approach.

@ArmorDarks
Copy link

But this is problematic if your peer project is hoisting dependencies

I meant that it's having same limitations with such (fortunately, quite rare) dependencies, but still a more transparent solution

using Eyeglass which I think would stipulate that the peer project must also use/setup Eyeglass

Think of Eyeglass as of NPM for Node. Not exactly the same things, but a separation of concerns should be noticeable. Sass should provide the fundament for modules loading, which it does, while Eyeglass implements modules resolution. It would be strange to expect Sass implement modules resolutions technics, since it isn't tied to any particular language, and thus can't take into account all possible modules managers.

@nex3
Copy link
Contributor

nex3 commented Apr 13, 2018

@yogeshgadge

IncludePaths means anything in the directory at any level deep. Here comes
you performance concerns:- if you have an import that does not exist in any
where in the includePaths you end up visiting the entire tree for each of
the values/directories of includePaths.

This isn't accurate. The includePaths option only refers to the top level of the path.

@penx I'm not sure what you mean by "peer project".

@yogeshgadge
Copy link
Author

yogeshgadge commented Apr 13, 2018 via email

@nex3
Copy link
Contributor

nex3 commented Apr 14, 2018

@yogeshgadge I think you're parsing that sentence slightly wrong—it says "paths that LibSass can look in to attempt to resolve", as in "paths that LibSass can look in in order to attempt to resolve". I think it's pretty clear that it means only the top-level directories, as that's how include paths work for most programming languages.

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

4 participants