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 Handlebars >2 #18

Closed
dschissler opened this issue Sep 21, 2014 · 11 comments · Fixed by #23
Closed

Support Handlebars >2 #18

dschissler opened this issue Sep 21, 2014 · 11 comments · Fixed by #23
Assignees

Comments

@dschissler
Copy link

handlebars-loader currently only supports handlebars < 2. Handlebars 2.0 was released on 2014-09-02.

@diurnalist diurnalist self-assigned this Nov 7, 2014
@iby
Copy link

iby commented Nov 12, 2014

Yes please! 👍

@kpdecker
Copy link
Contributor

I'm a bit unhappy with the progress that this project has on this front and generic handlebars use cases such as #17 as well as this. I'm contemplating implementing a webpack loader as part of Handlebars 3.x (My goal is to release this by the end of the year) but would love to hear feedback from both users and the maintainers here before I take that step.

My basic concerns are:

  1. This project ties into non-public Handlebars APIs so it has to be very careful with upgrades such as this.
  2. The assumptions are that all code is written fresh and neew for webpack
  3. There has not been a lot of activity on this front and there are a number of features that I'd like to implement (build time helper evaluation, etc) and I'm not sure that this project is being actively maintained.

I'd really like to avoid having yet another handlebars loader for webpack, but I also want to make sure that all Handlebars use cases are accounted for and users are not stuck on versions that are close to a year old.

@iby
Copy link

iby commented Nov 22, 2014

@kpdecker, it might not be maintained as much wished, but definitely being used a lot. But 👍 for not reinventing the wheel. I've been looking into this awhile ago, there's a lot of underwater rocks marrying both together.

#3 is kind of related and it has been partially addressed. For the sake of the past and the future releases, it would be ideal to decouple both completely and simply proxy every call to handlebars, addressing what's not working separately. But I feel if it could have been easily done, it would have been done this way from the beginning. Probably @altano can provide input on this.

Anything the rest of us can do to help you with this?

@altano
Copy link
Collaborator

altano commented Nov 23, 2014

I haven't had time to maintain this project for the last ~6 months which is when it started to get used a lot more. I gave @diurnalist commit writes because he was willing to fix some stuff up and I am willing to extend that same courtesy to anyone else who would like to help out. I'm also willing to review pull requests. If someone would like to fork the project and own it themselves, and they provide a superset of what this project supports, I'm also willing to shutter this one properly so that there's no confusion.

In short, you all tell me what you want out of this project and I'll do what I can.

There is a chance I'll get back into it at which point I'll first concentrate on the existing issues, but you shouldn't wait for me if it's frustrating you at all.

@diurnalist
Copy link
Collaborator

@kpdecker, any reason why you would rather fork than simply submit a pull request? I don't see how we wouldn't be able to make this module address the myriad of use-cases of Handlebars, or keep the dependencies fresh.

To your specific points:

This project ties into non-public Handlebars APIs so it has to be very careful with upgrades such as this.

Where is this criticism coming from? The Handlebars compiler is indeed a public API, and you are free to extend it, as this module does.

The assumptions are that all code is written fresh and neew for webpack

I'm assuming you're referring to our discussion on #17, which I haven't had time to reply to (sorry about that.) I'll go ahead and drop some more ink on that PR after I'm done responding here, but the long and short of it is, yeah it is a valid complaint that you have to work around code that is using Handlebars.registerHelper. We can and should address that, I just want to make sure it's done in a smart way, and doesn't increase the API surface more than necessary so we don't rethink things later and have to introduce breaking changes.

There has not been a lot of activity on this front and there are a number of features that I'd like to implement (build time helper evaluation, etc) and I'm not sure that this project is being actively maintained.

Hopefully you have more confidence that there are people interested in keeping this module alive and well. I can also help review PRs. The only thing I can't do is publish new versions of the package on npm.

Oh, and @ianbytchek, I didn't see that you had previously requested being able to link this loader to a different handlebars runtime library - that was added in #11 (I also commented on your original issue.)

@kpdecker
Copy link
Contributor

@diurnalist fork: I'm starting to use webpack for my own projects and also want to ensure that my developers have access to all of the latest handlebars features. Embedding a webpack loader in the library as a first class citizen that is lock step with the Handlebars library and tested as part of that is temping.

Private API: pushString is the offender in question. This is after the End Public API note. We do have an issue open to see what we can formalize as versioned APIs (most of the "internals" have not been versioned that well in the past, thus "private")

PR: Glad to look into PRs if they will be accepted. I must say that I was a bit turned off by the architectural comment but if this project will accept other use cases and we can keep it moving forward along with the Handlebars library itself, then not forking is likely a better option for the community.

@kpdecker
Copy link
Contributor

@diurnalist I have not had a change to look at impl here yet, but for Handlebars 3, we're going to try to move towards formalized AST-based API. https://github.com/wycats/handlebars.js/blob/master/docs/compiler-api.md

Might help avoid some of the overrides in this project and also should hopefully avoid the duplicate compile cases (via AST traversal for the analysis section). Feedback requested as this hasn't made it into a formal release yet.

@diurnalist
Copy link
Collaborator

@kpdecker thanks for the heads-up. From first glance, it looks like the AST api would be helpful for identifying 'known' helpers/partials, but we'd still need to override the compiler to insert webpack require statements where necessary, unless I'm mis-reading the intent of the AST Visitor.

@kpdecker
Copy link
Contributor

@diurnalist sorry for the delayed response. That's mostly true. Using the AST visitor lets you avoid having to parse the template multiple times which is the largest benefit since you are not using standard handlebars helper and partial lookup you will need to do the override for those portions.

There are changes that I can get into Handlebars 3 that make this easier for you, please let me know.

From my side, we ended up forking regardless as there were ecosystem specific concerns that we had to account for that would likely be very complicated to expose in a generic manner or to augment on the existing loader codebase:
https://github.com/walmartlabs/circus-handlebars/blob/master/lib/loaders/handlebars.js

@altano
Copy link
Collaborator

altano commented Dec 31, 2014

There's a lot of conversation to catch up here.

@kpdecker, you've forked the loader and are bundling it into what? handlebars itself? And so you're not not going to provide any PRs, right? Obviously it would be ideal to have a single loader that made everyone happy but if that didn't work out for your use cases that's obviously fine. That's what open source is for.

What does your version have going for it? If you were to release it publicly, can you please make sure it says what makes it different from this loader so that people can make an informed decision about what to use?

Other than that, is this issue back to being about supporting Handlebars 2? If so, this seems like the most pressing issue right now.

@kpdecker
Copy link
Contributor

@altano I "forked" but I did so in a separate project because I thought that it's goals were very different. In my day job (which is mostly separate from my handlebars job), I'm working on a system that builds on webpack for my teams needs, mostly one where you have linked libraries vs. built-in libraries, which is why we have a separate loader.

Basically the code I linked above does a number of things to to operate in the "CDN focused" system that we are building with our circus extensions and those are very different than most of the other webpack projects.

This loader right now is separate from the handlebars library and I don't intend to merge them (anymore, I had thought about it before I realized how domain specific our goals where).

Re: Handlebars 2 I'd mostly like to make sure that the users of both webpack and handlebars have access to the latest on both, as well as the upcoming 3 version which unfortunately has some breaking changes.

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 a pull request may close this issue.

5 participants