-
Notifications
You must be signed in to change notification settings - Fork 513
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
Add functionality for dynamic includes #93 #156
base: master
Are you sure you want to change the base?
Conversation
+1 Why this very old issue still has not solved by this solution? |
you'd have to pass it via options? seems a little weird, unless I'm misreading that, but it's not that simple, need caching etc, I haven't seen a good solid solution yet that's why there's no merge |
The var Edit: Seems like I'm wrong:
|
Is there a reason this can't be as simple as changing |
so, without this merge or one like it, is there a way to get dynamic include paths in ejs? I need to do includes based on a portion of the path being dynamic, and have wired ejs DEEP into my app's architecture. |
I'd be happy to merge this PR or one like it if it doesn't break existing tests. I'm taking a look at it now, buit the merge might go faster if the authors of the PR get tests passing before submitting it. |
Replaced path with templatePath as that is more semmantic and lifted the templatePath definition out of the if statement so as to prvent future jshint errors.
@mde I've fixed this PR so that existing test pass while still providing the required functionality. If there are any other changes you'd like me to make, just let me know. :) |
@jasper-lyons I'm probably missing something, but I'm not clear on what that block of code with the switch is for in this last commit. I made a simpler change yesterday in the master branch which passes all the tests, and added a test for the variable-based include. Can you take a look at it and see if it handles what you'd say the uses-cases are for this? |
@jasper-lyons Ah, I was looking at the changes to the built file. Looks like your change is pretty much what I implemented in master. Can you verify if it works for you? |
I can verify that it does and passes all test for me. I added a simple test case here at line 304-308 with some fixtures, simply outlining an automated test for the use-case. I would be happy to work on future extensions to this functionality if required. |
@jasper-lyons, I am doing a version 2.0 (using my preexisting EJS implementation from Geddy and previous) that handles includes as a simple function call at runtime: https://github.com/mde/ejs Could you take a look at it and let me know if it works for you? I believe I've gotten parity with all the existing functionality -- except filters, which I'd really like to deprecate. |
As per the issue raise #93 and with some amendments to corrects the suggest fix. Variables can now be used with includes so that the contents of the variable is evaluated to a string and then the include statement is evaluated as normal with the value of the string as the file name (without extension or path) to search for.