jstransformers / jstransformer-nunjucks

Nunjucks support for JSTransformers.
http://npm.im/jstransformer-nunjucks
MIT License
6 stars 6 forks source link

feat: add support for async compiling #42

Open gijswijs opened 4 years ago

gijswijs commented 4 years ago

jstransformer-nunjucks doesn't support asynchronous rendering, but nunjucks does, as explained in issue #35.

The basic jstransformer library looks for compileAsync, just before it defaults to the compile method as a final resort when compileFileAsync is called. This PR implement compileAsync

Nunjucks uses callbacks for asynchronous support, but jstransformer expects promises. Therefor the render function is denodified, which means that the render functions is wrapped in a promise, and the callback resolves the promise.

ci-reporter[bot] commented 4 years ago

The build is failing

✨ Good work on this PR so far! ✨ Unfortunately, the Travis CI build is failing as of c58982ab7f859bcd8135ec4a97f82ad52f4adbb6. Here's the output:

npm test
> jstransformer-nunjucks@1.0.0 test /home/travis/build/jstransformers/jstransformer-nunjucks
> test-jstransformer

 • nunjucks
   ✓ transform has an output format (0ms)
   ✓ transform has input formats (0ms)
   • basic
     ✓ nunjucks.compile() (5ms)
     ✗ nunjucks.compileAsync() (1ms)

       TypeError: template(...).trim is not a function
           at checkFunctionOutput (/home/travis/build/jstransformers/jstransformer-nunjucks/node_modules/test-jstransformer/index.js:88:38)
           at transform.compileAsync.then.template (/home/travis/build/jstransformers/jstransformer-nunjucks/node_modules/test-jstransformer/index.js:117:11)
           at tryCallOne (/home/travis/build/jstransformers/jstransformer-nunjucks/node_modules/promise/lib/core.js:37:12)
           at /home/travis/build/jstransformers/jstransformer-nunjucks/node_modules/promise/lib/core.js:123:15
           at flush (/home/travis/build/jstransformers/jstransformer-nunjucks/node_modules/asap/raw.js:50:29)
           at _combinedTickCallback (internal/process/next_tick.js:132:7)
           at process._tickCallback (internal/process/next_tick.js:181:9)

Total duration 15ms
tests failed

I'm sure you can fix it! If you need help, don't hesitate to ask a maintainer of the project!


Failed build for d427f8f ##### `npm test` ``` > jstransformer-nunjucks@1.0.0 test /home/travis/build/jstransformers/jstransformer-nunjucks > test-jstransformer • nunjucks ✓ transform has an output format (0ms) ✓ transform has input formats (0ms) • basic ✓ nunjucks.compile() (6ms) ✗ nunjucks.compileAsync() (1ms) TypeError: transform.compileAsync(...).then is not a function at TestCase.test [as fn] (/home/travis/build/jstransformers/jstransformer-nunjucks/node_modules/test-jstransformer/index.js:116:55) at /home/travis/build/jstransformers/jstransformer-nunjucks/node_modules/testit/lib/suite.js:74:29 at tryCallOne (/home/travis/build/jstransformers/jstransformer-nunjucks/node_modules/promise/lib/core.js:37:12) at /home/travis/build/jstransformers/jstransformer-nunjucks/node_modules/promise/lib/core.js:123:15 at flush (/home/travis/build/jstransformers/jstransformer-nunjucks/node_modules/asap/raw.js:50:29) at _combinedTickCallback (internal/process/next_tick.js:132:7) at process._tickCallback (internal/process/next_tick.js:181:9) Total duration 15ms tests failed ```

This comment was automagically generated by ci-reporter. If you see a problem, open an issue here.
gijswijs commented 4 years ago

I think the tests are wrong. The test assumes that template.fn(locals) returns a string. Hence the test fails when that functions returns a promise, because template.fn(locals).trim() returns an error. By wrapping it in a Promise.resolve() both the situations where template.fn(locals) returns a string or a Promise are covered. This is also how the jstranformer library works, so the tests now better mimick the behavior of the actual library. The test-jstransformer PR also adds a switch for async testing.

I also made a PR for the tests: https://github.com/jstransformers/test-jstransformer/pull/50 If that one gets merged, we can go ahead and see if this passes.