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

Support for Node.js file resolve based on require.resolve #168

Closed
rdmurphy opened this issue Nov 4, 2019 · 3 comments
Closed

Support for Node.js file resolve based on require.resolve #168

rdmurphy opened this issue Nov 4, 2019 · 3 comments

Comments

@rdmurphy
Copy link

rdmurphy commented Nov 4, 2019

Hello! 👋

I was wondering if you'd consider support (either via a flag, or something else!) for allowing file resolution on the Node.js side to be optionally based on require.resolve. What this would enable is the ability to provide paths to the {% layout %} and {% include %} tags that are able to find templates that may have been installed via npm. What's kind of cool about that is that you can maintain a separate project just for templates and such that could easily be imported into a project. Earlier this year Nunjucks added similar functionality.

I do think this could be accomplished easily enough by providing a custom fs implementation, but it's unfortunate that a user would be required to essentially copy all of the existing Node implementation only to alter the resolve function.

If there are any other easier paths I may be overlooking, please let me know! I've been using Nunjucks for a while but increasingly keep coming back to Liquid JS and this is one of the few features I'd miss out on.

Thank you!

@harttle
Copy link
Owner

harttle commented Nov 4, 2019

It seems the root option is not enough to cover this situation. Maybe we need another option for this, or it can be implemented as a fallback when no templates found in root directories, in which case it'll be backward-compatible and the additional option is not needed.

Another concern is require is not provided by browsers, so the implementation should go into fs/node.ts and be checked in parseFile when trying to call it.

Feel free to file a PR if you're already digging into it, I can help with the test cases and doc.

harttle pushed a commit that referenced this issue Nov 7, 2019
# [9.3.0](v9.2.0...v9.3.0) (2019-11-07)

### Features

* support require.resolve for lookup, see [#168](#168) ([2dd4355](2dd4355))
@harttle
Copy link
Owner

harttle commented Nov 7, 2019

Now it's implemented as a fallback so if no matching template is found under root directories, require.resolve will apply.

@harttle harttle closed this as completed Nov 7, 2019
@rdmurphy
Copy link
Author

rdmurphy commented Nov 7, 2019

Awesome, thank you! 🙌

Sorry I didn't respond earlier — got suddenly swamped this week. I think it was a good call to do it after an attempted local search too! That was going to be my main suggestion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants