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

v1.x.x #2

Open
wants to merge 21 commits into
base: master
Choose a base branch
from
Open

v1.x.x #2

wants to merge 21 commits into from

Conversation

chrisfarms
Copy link
Owner

@chrisfarms chrisfarms commented Feb 17, 2018

What

We want this module to have a decent set of features and full test coverage so we can we happy with using it in production build tools and so we have a base to build on.

  • Templates are precompiled where possible by default.
  • When the loader detects that it is not the first loader in the chain it
    will fallback to non-precompiled templates.
  • When non-compiled templates are in use the non-SLIM version of the
    runtime will be bundled.
  • Bundling for target:web should work for browser
  • Bundling for target:node should work for server
  • extend test
  • macro test
  • include test
  • import from test
  • import test
  • import from node_modules
  • setting Environment options
    • add filters
    • add globals
    • throwOnUndefined
    • lstripBlocks
    • trimBlocks
    • autoescape
    • tags
  • includePaths option should allow specifying paths to njk includes
  • nyc/istambul coverage tests
  • tests should fail when not 100% test coverage reported
  • precompile: [true | false | 'auto'] option (auto is the current behaviour, true=error if can't precompile, false=never precompile)
  • tests with precompile:false
  • travis should run the tests (including browser tests)
  • README documentation
  • port any tests/examples from nunjucks-loader
  • make options more compatible with nunjucks-loader (ie config vs configure option)

What is not being support yet

  • async filters - not had a use for them yet so haven't investigated much
  • env.express(app) - it's not clear how this should work and I don't think the magic res.render() is a good idea anyway.

How to review

Code review, check tests and coverage, try to use it based on the info in the README

We want to be able to test various configuration scenarios to ensure
that the loader works in different setups and different environments.

A _very_ basic test framework was created to allow a set of example
configurations to be tested, and these are build against both
target:node (and tested in ndoe) and target:web (tested via browser-run)

Notable changes:

* Templates are precompiled where possible by default.
* When the loader detects that it is not the first loader in the chain it
will fallback to non-precompiled templates.
* When non-compiled templates are in use the non-SLIM version of the
runtime will be bundled.
* Bundling for target:web now works as expected.
chrisfarms added 17 commits March 4, 2018 18:28
Use the schema-utils package to perform some basic schema validation of
the options object
Add a new 'configure' loader option that points to the location of a
module that exports a single (default) function that will be used to
make modifications to the Environment[1] instance that is used to render
any loaded templates.

For example adding:

```
options: {
  configure: 'path/to/nunjucks.config.js'
}
```

might cause the follow configure func to add filters/globals:

```
export default function(env) {
  env.addFilter(...);
  env.addGlobal(...);
}
```

[1] https://mozilla.github.io/nunjucks/api.html#environment
Enable configuring the "tags" (the characters used as the start/end
markers in templates. The tags option needs to be configured in the
environment at compile time. We support both precompiling and runtime
compiling so need to ensure the options are passed to all instances of
the environment.
Add test to ensure that autoescape is always set by default and enable
overriding it from loader options.
throwOnUndefined can be used to raise errors for any undefined variables
in templates (rather than the default of ignoring them). This adds the
loader option to configure this.
trimBlocks can be set to remove trailing newlines from a block/tag
the lstripBlocks option can be set to strip leading whitespace from
block lines. It's disabled by default.
Show how to use the configure function to set global variables on the
environment.

Since this test is basically the same as the custom filter test we also
use a different module syntax to try and catch any issues with how the
configure function/module is imported by the loader
import blocks should be discovered from includePaths
precompile: 'auto' - this is the default value, it will try it's best to
precopmile templates and bundle the 'slim' version of the nunjucks
runtime. Most of the time this is what you want.

precompile: true - if this is enabled it will throw an error during
webpack-time if it is unable to precompile templates and use slim. This
option is useful to protect against accidently introducing webpack
configuration that triggers the non-precompiled bundling which may be
important for browser bundles

precompile: false - if this option is set, precompilation is never
attempted and the full runtime is bundled. The only reason I can think
of that you might want this option is if you want to post process
nunjucks with some kind of loader that expects the raw source.
If you are attempting to import a njk template from a node_module ie

```
{% from "my-module/x.njk" import y %}
```

then the parentPath given to the template Loader would be a relative
path and not the expected absolute path required to reference the
template.

To work around this, we pass the expected name when compiling the
compiled ahead of time so the paths match.
We want to enable code coverage reporting to highlight any areas that
are not getting covered.

It's tricky to test some of the exceptions that get raised with the
current test setup so we ignore most of edge case exceptions since the
output is a fatal bail anyway.
To ensure consistancy between precompiled/runtime-compiled templates the
root template code and Loader were refactored so the ?mode=compile
always returns the expected src object with the correct env set.

This resovles issues with mixing compiled and uncompiled templates.
Use the test-with-coverage script to get coverage warning via travis
chrisfarms added 2 commits March 4, 2018 20:11
Bit of help for when you want to only run a single test by number.

```
TEST_FOCUS=013 npm test
```

to just build/run test 013
Add an example of adding a filter when precompile:false to prevent
regressions.

The filter is an example of using the jmespath library to allow
reshaping data objects in a declarative way.
@javipuche
Copy link

Release Date?

Thanks

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

Successfully merging this pull request may close these issues.

None yet

2 participants