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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ability to disable whitespace control: `Handlebars.parseWithoutProcessing` #1584

Merged

Conversation

@rwjblue
Copy link
Contributor

@rwjblue rwjblue commented Oct 25, 2019

When authoring tooling that parses Handlebars files and emits Handlebars files, you often want to preserve the exact formatting of the input. The changes in this commit add a new method to the Handlebars namespace: parseWithoutProcessing. Unlike, Handlebars.parse (which will mutate the parsed AST to apply whitespace control) this method will parse the template and return it directly (without processing 馃槈).

For example, parsing the following template (feel free to review the AST on ASTExplorer):

 {{#foo}}
   {{~bar~}} {{baz~}}
 {{/foo}}

Using Handlebars.parse, the AST returned would have truncated the following whitespace:

  • The whitespace prior to the {{#foo}}
  • The newline following {{#foo}}
  • The leading whitespace before {{~bar~}}
  • The whitespace between {{~bar~}} and {{baz~}}
  • The newline after {{baz~}}
  • The whitespace prior to the {{/foo}}

When Handlebars.parse is used from Handlebars.precompile or Handlebars.compile, this whitespace stripping is very important (these behaviors are intentional, and generally lead to better rendered output).

When the same template is parsed with Handlebars.parseWithoutProcessing none of those modifications to the AST are made. This enables "codemod tooling" (e.g. prettier and ember-template-recast) to preserve the exact initial formatting.

Prior to these changes, those tools would have to manually reconstruct the whitespace that is lost prior to emitting source.

@scalvert
Copy link

@scalvert scalvert commented Oct 25, 2019

This will definitely address a pain point WRT recast and whitespace, that I myself have definitely encountered. Thanks for this. 馃檹

@nknapp
Copy link
Collaborator

@nknapp nknapp commented Oct 25, 2019

I can see the need, but I don't think that a compile-option is the right way to go here. A compile-option would suggest that the use-case is compiling and executing a template with whitespace-control disabled.

But all you want is to build a clean, unmodified AST, correct?

I think a second "parse" function should be exposed for the purpose of building tools. Maybe there are other modifications that are useful for tooling, that could be built into the second function.

Consequently, the test-cases should not be along the lines of "shouldCompileTo" but more like the tests in the "ast.js"-spec: Build an AST and compare it to the expected one.

Does that make sense?

@rwjblue
Copy link
Contributor Author

@rwjblue rwjblue commented Oct 25, 2019

I don't think that a compile-option is the right way to go here. A compile-option would suggest that the use-case is compiling and executing a template with whitespace-control disabled.

The PR adds the option to parse, not compile, and the types call this object ParseOption, (which is different from CompileOptions). Am I thinking about the structure incorrectly? FWIW, I do find it very odd that handling whitespace control is part of "parsing" (as opposed to compiling). I just don't know how to fix that as it's a bit of a long standing thing.

I think a second "parse" function should be exposed for the purpose of building tools. Maybe there are other modifications that are useful for tooling, that could be built into the second function.

Sure, that seems totally fine to me as well. What do you think it should be called? Where should it go?

Consequently, the test-cases should not be along the lines of "shouldCompileTo" but more like the tests in the "ast.js"-spec: Build an AST and compare it to the expected one.

Sounds good, thank you for reviewing!

@rwjblue
Copy link
Contributor Author

@rwjblue rwjblue commented Oct 25, 2019

@nknapp - I just pushed another commit that separates parse into two functions (one that parses the input template string, and the other that leverages the raw parse output and adds whitespace control), as well as adding a number of additional tests to spec/ast.js.

Is that along the lines of what you were thinking? Do you have a better idea on the name of the (currently named parseAST) function?

@nknapp
Copy link
Collaborator

@nknapp nknapp commented Oct 27, 2019

How about parseWithoutProcessing

But "raw" is already taken for {{{{raw-blocks}}}} {{{{/raw-blocks}}}}. This might be confusing.
I am not sure about this either. The name might be only temporary, because we have plans to extract the parser into a separate package. When we do that, we can choose proper names for the function or even leave the white-space processing out of the package completely.

But we don't have a timeline. I can't make one with the small amount of work I can put into this every day.

Copy link
Collaborator

@nknapp nknapp left a comment

I have added a review (this makes the current state more visible in the PR overview).

lib/handlebars/compiler/base.js Outdated Show resolved Hide resolved
types/index.d.ts Outdated Show resolved Hide resolved
@rwjblue
Copy link
Contributor Author

@rwjblue rwjblue commented Oct 27, 2019

Thanks for your time @nknapp! I've updated with your latest round of feedback:

  • Rename method to parseWithoutProcessing
  • Update types for new method
  • Add types/test.ts for new API
spec/ast.js Show resolved Hide resolved
@rwjblue
Copy link
Contributor Author

@rwjblue rwjblue commented Oct 27, 2019

OK, just pushed another couple of changes:

  • Added documentation for parseWithoutProcessing to docs/compiler-api.md
  • Add explicit tests to spec/ast.js for using whitespace control (with parse and parseWithoutProcessing)
@nknapp
Copy link
Collaborator

@nknapp nknapp commented Oct 27, 2019

Thank you. I will have another glance but all in all the PR looks good to me now.

Copy link
Collaborator

@nknapp nknapp left a comment

One single change, then we are done...

types/test.ts Outdated Show resolved Hide resolved
@nknapp nknapp added this to the 4.5 milestone Oct 27, 2019
When authoring tooling that parses Handlebars files and emits Handlebars
files, you often want to preserve the **exact** formatting of the input.
The changes in this commit add a new method to the `Handlebars`
namespace: `parseWithoutProcessing`. Unlike, `Handlebars.parse` (which
will mutate the parsed AST to apply whitespace control) this method will
parse the template and return it directly (**without** processing
馃槈).

For example, parsing the following template:

```hbs
 {{#foo}}
   {{~bar~}} {{baz~}}
 {{/foo}}
```

Using `Handlebars.parse`, the AST returned would have truncated the
following whitespace:

* The whitespace prior to the `{{#foo}}`
* The newline following `{{#foo}}`
* The leading whitespace before `{{~bar~}}`
* The whitespace between `{{~bar~}}` and `{{baz~}}`
* The newline after `{{baz~}}`
* The whitespace prior to the `{{/foo}}`

When `Handlebars.parse` is used from  `Handlebars.precompile` or
`Handlebars.compile`, this whitespace stripping is **very** important
(these behaviors are intentional, and generally lead to better rendered
output).

When the same template is parsed with
`Handlebars.parseWithoutProcessing` none of those modifications to the
AST are made. This enables "codemod tooling" (e.g. `prettier` and
`ember-template-recast`) to preserve the **exact** initial formatting.
Prior to these changes, those tools would have to _manually_ reconstruct
the whitespace that is lost prior to emitting source.
@rwjblue rwjblue force-pushed the rwjblue:allow-disabling-whitespace-control branch from 1f1e071 to b58a228 Oct 27, 2019
@rwjblue rwjblue changed the title Add ability to disable whitespace control. Add ability to disable whitespace control: `Handlebars.parseWithoutProcessing` Oct 27, 2019
@rwjblue
Copy link
Contributor Author

@rwjblue rwjblue commented Oct 27, 2019

Just pushed another round of updates:

  • Fixed issues in types/test.ts
  • Squashed commits down to one
  • Updated description and commit messages to match current implementation

@nknapp - Please let me know if there is anything else that needs some tweaks. 馃樃

@nknapp
nknapp approved these changes Oct 27, 2019
Copy link
Collaborator

@nknapp nknapp left a comment

Thanks a lot for your patience and persistence in this.

I haven't decided on the date of the next release yet. Is this urgent for you or can it wait another couple of weeks?

@nknapp
Copy link
Collaborator

@nknapp nknapp commented Oct 27, 2019

I would have done the squashing myself, but well OK.

I noticed that unsquashed commits are much easier to review.

@rwjblue
Copy link
Contributor Author

@rwjblue rwjblue commented Oct 27, 2019

I haven't decided on the date of the next release yet. Is this urgent for you or can it wait another couple of weeks.

There are some bugs that are a bit difficult to work around in the codemod tooling that I am maintaining (things like ember-template-lint/ember-template-recast#155) that this helps resolve (once updated in @glimmer/syntax). Now that I know how the future versions of handlebars will support this, I can probably do a (slightly hacky) work around while we wait for 4.5.0.

tldr; I think I can "polyfill" this new method in my codemod tooling, if I run into issues there though I will let you know...

@nknapp nknapp merged commit 62ed3c2 into handlebars-lang:4.x Oct 27, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@rwjblue
Copy link
Contributor Author

@rwjblue rwjblue commented Oct 27, 2019

I would have done the squashing myself, but well OK.

Gotcha. I also wanted to fix up the commit message a bit (the main commit message was still talking about the original option I proposed).

I noticed that unsquashed commits are much easier to review.

Yes, I whole heartedly agree. The GitHub UI makes reviewing the delta between commits super simple, but when folks continually squash / amend that workflow is broken. FWIW, I have had some luck with the new "mark file as viewed" functionality in the GitHub pull request UI (this works across squashes), but it's still not as nice as leaving it unsquashed.

@nknapp
Copy link
Collaborator

@nknapp nknapp commented Oct 28, 2019

Before you start building workarounds... I would like wait for #1583 to be ginishrd. Then I could probably release 4.5 on the next weekend.

It won't be more than a couple of weeks at most. Maybe you can use a github dependency until then.

@nknapp
Copy link
Collaborator

@nknapp nknapp commented Oct 28, 2019

If you want this for ember, you probably need a backport to 3.x, don't you?

@nknapp
Copy link
Collaborator

@nknapp nknapp commented Oct 28, 2019

I might be able to release this evening. Do you know if you need the feature in 4.x or 3.x?

@rwjblue
Copy link
Contributor Author

@rwjblue rwjblue commented Oct 28, 2019

If you want this for ember, you probably need a backport to 3.x, don't you?

We are using v4 over in @glimmer/syntax (which is what Ember uses for parsing).

It won't be more than a couple of weeks at most.

Ya, definitely no worries on timing from me. Thank you again for all of your help getting this into a good state.

@nknapp
Copy link
Collaborator

@nknapp nknapp commented Oct 28, 2019

We are using v4 over in @glimmer/syntax (which is what Ember uses for parsing).

Really? A while ago @wycats asked for my opinion on how to proceed with the parser. He said Ember was using an older version and he was thinking about how to converge the code-base again, because Handlebars has features that are not used by Ember and Ember has other features like components.

But OK. It's a good to know that you are using the current version.

@nknapp
Copy link
Collaborator

@nknapp nknapp commented Oct 28, 2019

I have release 4.5 now. It was just good to have all this wrapped up. (will take a break now).

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

Successfully merging this pull request may close these issues.

None yet

3 participants
You can鈥檛 perform that action at this time.