Make CSS assets async

[fathyb]

Remove deasync from CSS assets.
Fixes #1331 and bring some performance improvements.

Status

• Sass
• Less
• Stylus

[DeMoorJasper]

What about creating: Resolver.resolveSync inside the Resolver, that would remove all deasync calls instantly (except for get package in Asset.js, but that's just for backwards compatibility)?

The changes in sass resolver is a good edit though, as it's async anyway it doesn't need resolveSync.

[fathyb]

@DeMoorJasper oh yeah, I was thinking the same. What do you think of making three classes :

• Resolver: the commons stuffs shared
• AsyncResolver: async implementation
• SyncResolver: sync implementation

I'm trying to find a good pattern where we could reuse maximum code and only have to implement fs stuff in the Resolver implementations.

What do you think of implementing a Thenable interface for each resolver :

interface Thenable<T> {
  then<U>(
    do: (v: T) => (Thenable<U> | U),
    catch: (err: Error) => (Thenable<U> | U)
  ): Thenable<U>
}

interface SyncThenable<T> extends Thenable<T> {
  get(): T

  then<U>(
    do: (v: T) => (SyncThenable<U> | U),
    catch: (err: Error) => (SyncThenable<U> | U)
  ): SyncThenable<U>

  static from(value: T): SyncThenable<T>
}

And in each resolver:

• SyncResolver.js

class SyncResolver extends Resolver {
  stat(file) {
    return Thenable.from(fs.statSync(file))
  }

  readFile(file) {
    return Thenable.from(fs.readFileSync(file))
  }

  resolve(input, parent) {
    // Returns the sync result of the Thenable
    return super.resolve(input, parent).get()
  }
}

• AsyncResolver.js

class AsyncResolver extends Resolver {
  stat(file) {
    return fsUtils.stat(file)
  }

  readFile(file) {
    return fsUtils.readFile(file)
  }
}

[DeMoorJasper]

I think the resolver should contain them both as a function, so that in the bundler we don't have to create a second instance if we would ever need a synchronous version.

[devongovett]

hmm this was originally added for Stylus, which requires the resolution be synchronous. Even if we switched to readFileSync etc. it would still block the event loop. I wanted to avoid having two completely separate implementations of the resolver for async and sync, and since this has the same effect it seemed ok to me. If you can figure a way to make the Stylus resolution happen asynchronously without too much hassle that would be better, but until then deasync seemed like the easiest way.

[DeMoorJasper]

Hope you don't mind me committing to this branch but I found a way to make the less filemanager async and pushed it to this branch @fathyb

So apparently all that's left now is Stylus

[codecov-io]

Codecov Report

Merging #1390 into master will increase coverage by 3.49%.
The diff coverage is 85.91%.

@@            Coverage Diff             @@
##           master    #1390      +/-   ##
==========================================
+ Coverage   85.01%   88.51%   +3.49%     
==========================================
  Files          79       80       +1     
  Lines        4391     4639     +248     
==========================================
+ Hits         3733     4106     +373     
+ Misses        658      533     -125

Impacted Files	Coverage Δ
src/FSCache.js	98.24% <100%> (ø)	⬆️
src/utils/pipeSpawn.js	92% <100%> (-0.31%)	⬇️
src/assets/SASSAsset.js	79.1% <60%> (-12.13%)	⬇️
src/assets/LESSAsset.js	92.53% <85.71%> (-0.52%)	⬇️
src/assets/StylusAsset.js	91.73% <92.5%> (+4.67%)	⬆️
src/utils/syncPromise.js	57.89% <0%> (-42.11%)	⬇️
src/utils/loadPlugins.js	64.28% <0%> (-26.2%)	⬇️
src/assets/PugAsset.js	88.57% <0%> (-11.43%)	⬇️
src/utils/PromiseQueue.js	90.78% <0%> (-3.45%)	⬇️
... and 36 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 08f9389...c754250. Read the comment docs.

[fathyb]

Awesome @DeMoorJasper, I don't mind at all, thanks!

For Stylus I think I found a way to make it async using a double-traversal :

• Parse the Stylus AST using stylus.Parser
• Use a stylus.Visitor to traverse nodes and collect dependencies
• pre-resolve and await dependencies promises
• Call the custom stylus.Evaluator using the dependencies we already have resolved

[fathyb]

Just pushed an async StylusAsset, no need for a sync Resolver anymore 😄

[dobesv]

I tried it out and it ran to completion for me.

[jpergler]

I can also confirm that the build finishes on previously problematic project.

[devongovett]

unfortunately I think this will result in the code being parsed twice for each file: once here, and once when actually processing the code later. Do you think there is a way they could share the same AST?

[fathyb]

I tested by putting some logs in Stylus' code and (ironically) cache: true broke the Parser cache (the cache is disabled if it isn't 'memory').

I fixed it and now it'll parse in getDependencies and use the cached AST in generate.