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

multiple namespaces fix #580

Merged
merged 2 commits into from
Nov 18, 2018
Merged

multiple namespaces fix #580

merged 2 commits into from
Nov 18, 2018

Conversation

stefspakman
Copy link

Fix to allow multiple namespaces

@dave-irvine
Copy link
Contributor

Hi @stefanullinger , thanks for your contribution. Could you add a unit test that fails before your change and passes afterwards?

@stefspakman
Copy link
Author

Hi @dave-irvine,
I just tried making a testcase for this fix, however the current multiple namespaces test already seems to be passing every time. As i do not have any experience in writing unit tests, i'm not sure how to best recreate my case as a test.

The problem I had were as follows:
when i declared multiple namespaces, only the first one would actually work, any other would not be picked up by the render function.

Twig.twig({
id: id,
data: twig,
namespaces: {
'atoms': path.resolve(__dirname, ${process.cwd()}/patterns/atoms/),
'molecules': path.resolve(__dirname, ${process.cwd()}/patterns/molecules/)
}
}).render(json)

when my twig uses {% include "@molecules/testfile.twig"%}, it won't find the file because it looks at /patterns/atoms/testfile.twig, instead of patterns/molecules/testfile.twig

@RobLoach
Copy link
Collaborator

Seems like this is more of a performance update? It says if the namespace is not found in the given file, skip it. In theory, tests should pass with or without this change.

src/twig.path.js Outdated
@@ -27,6 +27,9 @@ module.exports = function (Twig) {

if (hasNamespaces){
for (k in namespaces) {
if (file.indexOf(k) === -1) {
continue
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be 3 spaces instead of 2.

@stefspakman
Copy link
Author

Hi @RobLoach,
Yes, maybe I should have classified it as an performance update. The issue occurred when processing a lot of files, with this change it should process the namespaces faster.

Copy link
Collaborator

@RobLoach RobLoach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like a good change that doesn't affect tests negatively 👍

@RobLoach RobLoach merged commit 6f36ad1 into twigjs:master Nov 18, 2018
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.

3 participants