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

Add VM TS mapping #1534

Merged
merged 2 commits into from Sep 3, 2019
Merged

Add VM TS mapping #1534

merged 2 commits into from Sep 3, 2019

Conversation

@AndrewLeedham
Copy link
Contributor

@AndrewLeedham AndrewLeedham commented Jul 3, 2019

I am aware VM is not part of the public API. But, In one of our projects we are overridding the resolvePartials function in order to do some custom resolution, it would therefore be useful if it was mapped to TS to aid in that.

Not sure if these typings are 100% accurate, I'm not particularly familiar with the structure of handlebars.js behind the scenes, but I took my best stab at it.

  • Please don't start pull requests for security issues. Instead, file a report at https://www.npmjs.com/advisories/report?package=handlebars
  • Maintain the existing code style
  • Are focused on a single change (i.e. avoid large refactoring or style adjustments in untouched code if not the primary goal of the pull request)
  • Have good commit messages
  • Have tests
  • Have the typings (lib/handlebars.d.ts) updated on every API change. If you need help, updating those, please mention that in the PR description.
  • Don't significantly decrease the current code coverage (see coverage/lcov-report/index.html)
  • Currently, the 4.x-branch contains the latest version. Please target that branch in the PR.
@AndrewLeedham
Copy link
Contributor Author

@AndrewLeedham AndrewLeedham commented Jul 15, 2019

@nknapp Does this seem right?

@AndrewLeedham
Copy link
Contributor Author

@AndrewLeedham AndrewLeedham commented Aug 30, 2019

@nknapp would be useful to know if this is likely to get merged? Perhaps the typings should be added back to DefinitelyTyped?

@zimmi88 what was the original reason for including the typings here?

@nknapp
Copy link
Collaborator

@nknapp nknapp commented Aug 31, 2019

I missed this one, sorry about that...

Honestly, I'm not sure if I want to support this. Adding those type definitions would make those internals an official API which means that we would not be able to change it without a major version increase.

On the other hand, the resolvePartials-method is mention in the compiler-api so it is already kind of official.

The compromise I would be willing to accept is:

  • Add VM with just the resolvePartials function and omit the other functions.
  • Add a /** @deprecated */ annotation to the function to clarify that this is not officially endorsed anymore.
  • Add a test to types/test.ts
  • Add a comment to the implementation of the function, warning that it is currently
    part of the official API and should not be changed.
  • open a new issue to discuss a new way for providing the functionality.

@AndrewLeedham
Copy link
Contributor Author

@AndrewLeedham AndrewLeedham commented Sep 1, 2019

@nknapp Makes sense, I have implemented your suggestions and opened a discussion issue: #1545

- Handlebars.VM is actually not part of the API,
  but Handlebars.VM.resolvePartial is mentioned
  in the documentation and is thus now treated
  as part of the API.

Closes handlebars-lang#1534
nknapp added a commit to AndrewLeedham/handlebars.js that referenced this issue Sep 3, 2019
- derived from the object structure seen in the debugger

closes handlebars-lang#1534
@nknapp
Copy link
Collaborator

@nknapp nknapp commented Sep 3, 2019

Thank you. I have made some adjustments. The "options" object of "resolvePartial" seems to be different from the RuntimeOptions. Some options are missing and the "name" is new. I have used the debugger to inspect the object and decided that it should be its own type.

I hope it was ok to force push on your branch. I neede to do that after squashing and rebasing the branch.

- derived from the object structure seen in the debugger

closes handlebars-lang#1534
@nknapp nknapp merged commit 8ac2028 into handlebars-lang:4.x Sep 3, 2019
1 of 2 checks passed
@nknapp
Copy link
Collaborator

@nknapp nknapp commented Sep 3, 2019

Released in 4.2.0

@AndrewLeedham
Copy link
Contributor Author

@AndrewLeedham AndrewLeedham commented Sep 3, 2019

Thank you. I have made some adjustments. The "options" object of "resolvePartial" seems to be different from the RuntimeOptions. Some options are missing and the "name" is new. I have used the debugger to inspect the object and decided that it should be its own type.

I hope it was ok to force push on your branch. I neede to do that after squashing and rebasing the branch.

That is fine, makes more sense now. Thanks for getting this in :)

@AndrewLeedham
Copy link
Contributor Author

@AndrewLeedham AndrewLeedham commented Sep 4, 2019

Actually @nknapp it seems that name does exist outside resolvePartials. See https://jsfiddle.net/AndrewLeedham/jkpb3equ/6/, it exists during compile if it is currently compiling a partial. So perhaps it should be optional on the RuntimeOptions interface?

@nknapp
Copy link
Collaborator

@nknapp nknapp commented Sep 5, 2019

The "name" is indeed set to the runtime options, when a partial is called. But I would say this is a collateral effect, not intended. Passing "name" to a compiled template does not have any effect, in the template as far as I can see.

Do you need the "name" to be in there?

@AndrewLeedham
Copy link
Contributor Author

@AndrewLeedham AndrewLeedham commented Sep 6, 2019

The "name" is indeed set to the runtime options, when a partial is called. But I would say this is a collateral effect, not intended. Passing "name" to a compiled template does not have any effect, in the template as far as I can see.

Do you need the "name" to be in there?

We are currently using it yes. It is an unusual use case, but we are checking the name against a map of Mustache templates and rendering it if it exists, otherwise just continue the normal handlebars render.

@nknapp
Copy link
Collaborator

@nknapp nknapp commented Sep 8, 2019

I can see how you are using it? Where do you insert the logic that checks the name against the template map. Are you overriding the "compile" function?

I'm asking because I don't think that this should be in the official API. Overriding any functions in the Handlebars-prototype is not intended and should not be promoted. Every addition to the API means that there is one more case where I would have to increase the major version of the library. But I would like to maintain compatibility, because that is least hassle for all users.

You can send me a short fiddle that shows how you are doing it. I may change my mind and add it. Please create another issue for that, so that it is better to find.

But after reading "Clean Code" by Robert C. Martin, and looking at the Handlebars source code, I feel the urge to do some refactorings. The code involving the "name" option. May also be subject to change in the future.

If you give me a usage example, I might keep it in mind when changing that code.

In any case, you can still add you own type definitions in your own projects, merging the "name" into the options.

https://www.typescriptlang.org/docs/handbook/declaration-merging.html

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

Successfully merging this pull request may close these issues.

None yet

2 participants