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

Make loader aware of "internal" partials #151

Merged
merged 6 commits into from
Jan 21, 2018

Conversation

andreyvolokitin
Copy link
Contributor

When partial is failing to be resolved there is an error thrown here, which interferes with the use of inline partials and failover content of partial blocks (#106, #135 ). Currently, handlebars-loader considers any partial names as external partials. But some partials with names not necessarily should be external (i.e. inline partials), or it may be acceptable to not have a resolved partial by this name if it is a partial block (which always has failover content).

I used AST Visitor to save names of any partial blocks and inline decorators in the current template. In case of resolve error, I check if the name of a partial from failed request is the one of those saved names and if that is the case I proceed with return partialCallback(); so that Handlebars can do all remaining work. In other case I pass error along as usual

As opposed to external partial which needs resolving, "internal" partial is defined in the same file (may be inline partial or failover content of unresolved partial block). If reference to it fails to resolve as external file, we need to check if the same file contains target partial
@coveralls
Copy link

Coverage Status

Coverage decreased (-3.4%) to 83.534% when pulling 35580ca on andreyvolokitin:internal-partials-resolving into 67657e2 on pcardune:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-3.4%) to 83.534% when pulling d742f53 on andreyvolokitin:internal-partials-resolving into 67657e2 on pcardune:master.

@pcardune
Copy link
Owner

pcardune commented Jan 9, 2018

Thanks for the pull request! Do you think you can add some unit tests for this fix?

@andreyvolokitin
Copy link
Contributor Author

I am actually not familiar with unit testing and testing in general. I read wiki on it a bit but still has no clue what to test and how... The only new "thing" is InternalBlocksVisitor, which is just an extension of Handlebars Visitor with minor and trivial changes

@andreyvolokitin
Copy link
Contributor Author

I will try it though

@coveralls
Copy link

Coverage Status

Coverage increased (+2.2%) to 89.157% when pulling f984f28 on andreyvolokitin:internal-partials-resolving into 67657e2 on pcardune:master.

@andreyvolokitin
Copy link
Contributor Author

Are my tests ok?

@pcardune pcardune merged commit a8a9773 into pcardune:master Jan 21, 2018
@pcardune
Copy link
Owner

Looks good! Thank you. I'll publish a new release in the next few days.

@aparshin
Copy link

@andreyvolokitin thank you for this PR!
@pcardune when are you going to publish new version?

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.

4 participants