feat(index): add noImporter option (options.noImporter)
#307
Conversation
|
Thx for your feedback @anchann I'm considering this change, although I don't think it's right to pull this in. We should rather try to tackle the perf issues. Unfortunately, I can confirm that a lot of time can be spend on resolving. I've published a resolve-perf-tests that uses 240 seconds to 3 seconds are extreme numbers. Can you give me some hints on your directory structure and how you import stuff? |
|
Thanks for taking a look, Johannes.
Fair enough. The only thing I wanted was that if somebody faces this problem, my findings are shared and indexed in google. The last few weeks of working on converting a large project from a three-year-old grunt-based toolchain to a webpack one has taught me one thing — none of this stuff works like magic out of the box, at the end of the day, you have to dig inside node_modules to get it working. It was the same story with the typescript part of the toolchain — takes forever, no output to shed light onto what's happening, yet calling tsc on the command line builds the project in seconds. I'd love to be proven wrong, but until then, logging my findings for the next dev is the best I can hope for :)
My personal opinion (which, I realize, is worth little) is that too much magic is a bad thing. Sass has since inception supported an includePaths configuration option specifically to make imports more readable and flexible. From this perspective, webpack's resolution seems to add nothing in terms of functionality, though perhaps it does add something in terms of uniformity (that said, uniform opaqueness is not necessarily a good thing). I'm actually using importPaths specifically to be able to import scss files from other node_modules, having it set to Also, importers are labelled as experimental in node-sass' documentation. Would it not make sense to make the use of an experimental feature optional, specifically because the upstream library labels it as a "use with caution" feature? I would personally go as far as making it disabled by default, and only enabled when a user knows that they explicitly want the feature.
I don't think that's the case — disabling the webpack importer leaves sass' native underscore resolution intact, does it not?
When debugging, I had a single These are provided by the The compass directory has on the order of 50 files in it. So all in we're looking at around 60 imports per file, which presumably means 60 resolve calls for 130 files, or around 8000 calls. That would make it something like 300 ms per call, which I'm pretty sure isn't what's happening. Yesterday when debugging I was dumping timings for resolution calls, and most were in the low tens of milliseconds, though about one in twenty clocked in at low hundreds. Let me know if this is enough information for you to go off of. If not, and you think it's worth the time, I'll hack the loader to add some timers to give a more concrete picture. Or, if it's easier for you, you can give me a custom version of sass-loader with whatever instrumentation you want, and I'll send you the output data. |
We already heard of builds taking minutes to finish. Unfortunately, we don't have complex example projects to replicate these issues. My projects usually finish after max. 20 seconds in production mode, but they're medium sized.
Since webpack provides its own resolving algorithm, I felt that it would be consistent for the developer to configure all resolving stuff in your webpack.config. It's true that this uniformity is not acceptable at the current performance cost, so I understand your argument here.
Our importer is well-tested and works without errors, so I don't see the need to declare this as an experimental thing.
Once we use our own importer, sass' native importing algorithm is not available. That's why we have to replicate it (and it's unnecessary complex 😢)
Could you use the sass-loader from the resolve-perf-tests branch and tell me the last output (it's accumulating all numbers)? |
|
Could you check if the import is still slow when you change Just set the environment variable before calling webpack. |
|
Apologies for the silence: after I finally finished the conversion work on my project, the pressure to work on user-facing features out-prioritized my feeble attempts at community involvement. To be honest, I didn't even want to do this test when I saw the variable name, since it sounded like a poor quality sledgehammer approach to the problem. I still firmly believe that even if resolving the problem internally inside webpack is the philosophically correct thing to do, given that lib-sass has a working solution to accommodate aliasing via import paths, sass-loader should provide its users with an escape hatch in case they want one. Either way, I refactored my project a bit to get back to a setup about which I originally posted, one with a top level scss file that imports 133 component-specific scss files. There were some library-specific files that I didn't bother with, so they'll show up stand-alone in the output. I do not know why webpack builds the whole thing twice — I didn't even know that it does before I put the instrumentation in. Instrumentation is basically the following timer: Here's what the numbers look like (resourcePath stripped for ease of reading/privacy). First, with the importer disabled (which is how I run it normally). Next, with the latest sass-loader from npm. Even though the calls go through sass-loaders async functions, it looked as if something was serializing the calls along the way. Upon closer inspection, this seems to also be happening in the no-importer version, but because the times are lower, it's less obvious. CPU was sitting at around 95% (out of a total of 400% available) the entire time. Finally, with the UV_THREADPOOL_SIZE set to 200, we do see a bunch of parallelization happening with the smaller files, but the big top level index of all components' scss still takes about the same amount of time. |
|
Thx for your efforts. This helps me a lot! 👍
No worries! This won't be the final solution. I just wanted to check if it might be a problem that node-sass eats up almost all available threads for file IO. There was a problem in libsass that caused a deadlock (#100 #147) which was the reason why we had to use async's job queue. This way, node-sass still leaves one thread available for node.js – which is also the only thread for webpack's file IO. Thus webpack can only resolve files sequentially. And I think your numbers confirm my assumption. For instance, take a look at
webpack's resolving engine is more powerful than node-sass' My current approach is:
|
You can make an aliases directory, symlink whatever you need into there, and add it to There is also another good reason to use each technology's native features, rather than something like webpack's resolver, that I recently ran into with TypeScript. There are various text editors that support things like code completion, display errors as you edit, let you jump to definition, etc. These tools typically don't know how to read webpack's config and tease out alias information, so as far as they are concerned, the import/require statements are broken and point to non-existing files. Much like sass, TypeScript happens to have its own system for managing aliases/includes, through a tsconf.json file, which most editors understand. So while webpack's goal of providing a resolving engine that works across the whole stack is admirable, it breaks other tooling. I think your argument would convince me in the case if webpack just worked, out of the box. But if for advanced use cases you need to dig into the code to get things working, it doesn't matter whether webpack provides advanced functionality, or whether you hack it yourself with symlinks or something — you spend the same amount of time. Now, I understand that your efforts are actually geared specifically at getting webpack to the point where everything just works out of the box. But I personally believe that this goal is impossible to achieve, because new versions of various technologies come out at about the same speed as it takes to polish off an out-of-the-box-working toolchain to support them. So by the time everything works great with webpack, there will be some new kid on the block, also as cool and must-switch-over-to, and as not-working-out-of-the-box for advanced use cases. This probably sounds cynical, but I've seen it happen enough times now to see a pattern. Though who knows, maybe we'll reach some kind of equilibrium point once a toolchain that satisfies most common use cases emerges. Anyway, sorry for rambling. It's a month since I finished the stack transition, and I got my momentum back, so now I can afford a few cycles here and there to at least give you metrics if you need them. So let me know if you need to test anything against my setup, whether the artificial one-big-file-with-many-imports, or the actual one that I use, where each components' .scss file is require'd at the top of the .ts file. |
|
I did some profiling: #296 (comment) |
|
Regardless of the performance concerns, there is definite user need to allow standard sass resolution semantics instead of webpack specific resolution. |
|
@anchann friendly ping, what is status? |
|
@evilebottnawi Is something blocked on me? Ever since I originally posted the question, I've been using my own hacked version with the webpack resolution disabled. |
|
Given this is performance related, @jhnns has the final say as to how & when this gets merged. |
michael-ciniawsky
added
status: Pending
pr_action: Rebase
and removed
pr_action: Discuss
status: Needs triage
labels
michael-ciniawsky
changed the title
noImporter option (options.noImporter)
michael-ciniawsky
changed the title
noImporter option (options.noImporter)noImporter option (options.noImporter)
|
In case no one objects within the next ~48 hours, I'm going ahead and declining this PR for now |
|
Closing due to inactivity. Thanks! |
Function calls associated with the advanced file resolution described above can add up to a significant time on projects with a large number of scss files. On a project with some 130 components each requiring an .scss file, sass build took 240 seconds with the importer, and 3 seconds without.
Everything below is my attempt at keyword bait for the next unfortunate soul dealing with this issue, as it took me half a day to figure out which part of the webpack pipeline is responsible for the slowness.
webpack 2, css-loader, sass-loader, style-loader, ExtractTextPlugin, extract-text-webpack-plugin, slow large project, many imports, require .sass from .js, super-slow, really slow, insanely slow