Add CJS option

[OliverJAsh]

No description provided.

[tkellen]

I want to support this, but I think we need to discuss the option naming in general for this task. The possible combinations of cjs/client/amd/namespace seem a bit messy.

Any thoughts on how to handle this?

/cc @shama @sindresorhus @cowboy

[OliverJAsh]

My thoughts: a single wrapper property, which accepts a string of cjs or amd, although I believe this used to be the case and was optionally changed. I'm not entirely sure how you can combine wrapper options with the client/namespace options. Hopefully you guys have some better thoughts!

[sindresorhus]

How about a type property which accepts either, html, js, commonjs, amd. Defaults to html.

Can anyone explain to me the point of the namespace property? Why is it there?

[tkellen]

If you compile multiple templates to a single file, namespace controls what variable they are stored under.

[shama]

A type property like @sindresorhus suggested sounds better. I wonder if there is an existing wrapping library out there that supports all of these types. Seems like a very common thing.

[tkellen]

@OliverJAsh would you be game for attempting to implement this as a helper in grunt-lib-contrib? I'd like to add support for this across all of our plugins if we're going to do it. Eventually, we'll be able to allow chaining output through multiple tasks, obviating the need for this sort of ad-hoc solution, but it's a ways off.

[OliverJAsh]

@tkellen I had a shot at implementing the API described above by @sindresorhus. I hope I'm heading in the right direction here… If I am, let me know and I'll try to abstract some of the options.type formatting to grunt-lib-contrib — if that's what you mean 😄

[tkellen]

@OliverJAsh It looks to me like you're headed in the right direction. I haven't thought this through completely, but the helper I have in mind would take the compiled template string, the mode, and perhaps the namespace as arguments and return the correct contents. That way we can re-use this logic across all of our templating tasks. Basically, this is a stopgap solution until we can pipe data between tasks (at which point we'd just have like, a single cjs wrapping task that could be part of any task pipeline).

[OliverJAsh]

@tkellen What do you think?

[OliverJAsh]

So the formatting for templates has been abstracted to formatForType with the function signature suggested by @tkellen. However, this task then wraps the complete templates in more formatting depending on the type. This will require another method in grunt-lib-contrib, but abstracting this will be awkward because this task inserts the Jade runtime as a dependency during the formatting. What are your suggestions?

[tkellen]

Sorry, it's been a crazy day here. I'll check on this further tomorrow Oliver. Thanks for taking the time to put this first draft together.

[tkellen]

@OliverJAsh I think it's fine to have this task do some wrapping internally in addition to what has been generalized out to lib-contrib. There is still a big win in being able to share the common case across multiple tasks.

[tkellen]

ping @OliverJAsh the changes to lib-contrib are live, if you want to move forward with this

[OliverJAsh]

@tkellen As far as I can tell, I have made the necessary changes to this task. Can you tell me what's missing? I can't see what can be done about the wrappers.

[tkellen]

@mereskin Can you explain why if(jade && jade['runtime'] !== undefined) { jade = jade.runtime; } is included in the AMD wrapper? If you want the jade runtime, shouldn't this be handled in your AMD config?

[mereskin-zz]

@tkellen This instruction is an assumption, that module, defined as jade may contain either jade itself or jade runtime. It also works, if you use requirejs within node environment.

[tkellen]

I can read the code--I'm asking why it is necessary.

[mereskin-zz]

@tkellen On the server requirejs("jade") returns jade. On the client same instruction returns jade.runtime. I used requirejs server-side to load templates for the tests.

I would agree that AMD config is a better place for that.

[tkellen]

@OliverJAsh We can remove the additional wrapping stuff. Would you be willing to do that? If so, I think we're all set!

[tkellen]

ping @OliverJAsh

[OliverJAsh]

@tkellen You want me to remove all of this wrapping code? I'm confused. Without this, AMD will not work, and CommonJS will not work if the namespace option is set. We still need the wrappers. I think I misunderstand — please explain. I'm not really sure how you would like me to move forward from here.

[tkellen]

I apologize for being so obscure--I haven't had the time to properly engage on this issue and I don't see that changing in the near future.

The idea is to have as much of the wrapping code come from a shared method (formatForType in grunt-lib-contrib) as possible. We do this exact stuff on numerous templating tasks. This is basically a hack to make things consistent across grunt-contrib until we can pass tasks through a pipeline a-la http://github.com/node-task/spec

Per our discussion above, this code is not needed at all: https://github.com/OliverJAsh/grunt-contrib-jade/blob/master/tasks/jade.js#L95
This code should be in grunt-lib-contrib: https://github.com/OliverJAsh/grunt-contrib-jade/blob/master/tasks/jade.js#L97

Here is a really rough sketch of what I'm thinking.

formatForType({
  string: template value here,
  type: 'amd' | 'commonjs' | 'js' | 'html' | 'whatever',
  namespace: 'namespace'
  filename: 'filename',
  header: "define(['blah'], function(blah) {",
  footer: "});"
});

If that isn't clear enough, please rebase this PR and I'll just merge it and move on--I don't have the bandwidth to champion getting this implemented :(

[OliverJAsh]

That's much clearer, thank you. I will give this a shot sometime in the next week, if that's okay. I shan't forget, though!

[tkellen]

perhaps we wouldn't even need header/footer and could instead have a list of dependencies like

formatForType{{
  ...
  deps: {
    handlebars: 'Handlebars',
    jade: 'jade',
  }
});

...which could expand out to load deps like
define(['handlebars'], function(Handlebars) { })

[tkellen]

Thanks Oliver!

[tkellen]

ping @OliverJAsh :)

[OliverJAsh]

@tkellen As far as I can tell, there is still an issue with the implementation you propose.

This code runs for every template (where formatForType is currently being used): https://github.com/OliverJAsh/grunt-contrib-jade/blob/patch-1/tasks/jade.js#L72

This code runs for every file: https://github.com/OliverJAsh/grunt-contrib-jade/blob/patch-1/tasks/jade.js#L85-107

Thus, there are two places where the formatting needs to take place, and I'm failing to see how this can be achieved in a single sweep. Hence why I've stumbled and stopped where I have.

Maybe it would be a good idea to list what all of the possibilites are. I have:

• AMD: one template to a file, exports function
• AMD: multiple templates to a file, exports object
• CommonJS: one template to a file, exports function
• CommonJS: multiple templates to a file, exports object

The items in bold are the ones which need formatting per file as well as per template. Can you see why formatting needs to happen in two places?

Sorry for the poor correspondence. Up until yesterday I was final year university student.

[tkellen]

@OliverJAsh This code: https://github.com/OliverJAsh/grunt-contrib-jade/blob/patch-1/tasks/jade.js#L85-107 can be removed per this comment: #19 (comment)

No worries about the slow reply--I'm swamped here too.

[OliverJAsh]

@tkellen Can you respond to my issues that I posted previously? Otherwise I think I'm no good here anymore.

[chrisspiegl]

+1

This should really be something in this package. Or is there any known workaround till this feature is implemented?

[chrisspiegl]

Sorry for the double post but here is a question I would like to ask about this feature:

There is a option in the code called options.node which, when set to true and client:true as well as namespace!==false will produce some kind of common js (module.exports) code which than can be used easily.

Is this different form what is asked for here? Is it a good workaround for now? And is the options.node not documented on the README.md on purpose? Thanks.

[vladikoff]

This is a long running PR and looks like sadly it won't get merged. We are still looking for the best way to either have a single module that takes care of CJS wrappers for all contrib plugins or at least something that is common between the plugins that takes care of such wrapping.