render now supports token arrays that parse-appcache-manifest generates

[mreinstein]

@meryn I've taken removed lib/ from .gitignore. If the lib directory isn't present and containing the source code for the module, anyone pointing at a github repo in their package.json will have a non-functioning library. (e.g., I'm using my fork of render and parse because the version in npm doesnt support tokens yet. Because lib/ was in my .gitignore,and because I'm referencing my github repo in package.json, it breaks.)

[myrne]

I've never really considered people referencing a git repo as dependency. I may need to reconsider my stance on not including built files. I'll accept this now to ease development for you.

I still feel there should be some different way of doing this. Perhaps npm could be taught to consume github "releases", but then this release process would have to be properly automated (as easy as doing npm version patch && npm publish). But this is merely wishful thinking, for now.

[edit: At least someone proposed something like this for Bower: https://github.com/bower/bower/issues/584 ]

Did you not include any tests or am I looking over them somehow? I think it's good to have at least something rudimentary.

Also, I think this code would not seamlessly work together with my proposed results object for parseManifest. That would be an object with cache,network,fallback and tokens property. I think that if input data is an object that contains a tokens object, renderManifest could ignore the other properties, to get output more faithful to original input. Now, if someone would do renderManifest(parseManifest(manifestContents)), renderManifest would ignore the tokens property.

I'm willing to accept this PR "as is", and make adjustments myself, but would like your comments on these two points.

[mreinstein]

I still feel there should be some different way of doing this.

I don't see it as a big problem, but that's just my opinion. If npm supports a post install hook that could be one way to fix it. But then you'd need to have coffee-script installed, and that seems even messier to require that dependency when it's only for the build step.

Did you not include any tests or am I looking over them somehow?

I didn't add any tests to the render package, but I added the tokenize output assertions in the parse package. Certainly willing to add some to the render package as well.

Also, I think this code would not seamlessly work together with my proposed results object for parseManifest.

It might just be that I don't understand, are you saying that you are unhappy with the fact that 2 slightly different manifest files will be produced depending on whether you are using an object or an array of tokens?

That would be an object with cache,network,fallback and tokens property

What I was hoping to avoid is having to pass in data to the render function that I know it will never use, but it's just required to be there. IMO it's cleaner to only pass in the data that the render function needs, and that clarity is also more apparent in the docs too.

[myrne]

But then you'd need to have coffee-script installed, and that seems even messier to require that dependency when it's only for the build step.

That's why I'd rather not go in that direction. I like the fact that npm contains the pure Javascript, and so should any releases. I just see a world of difference between a source code repository (for logging changes in source code) and a package repository (for downloading dependencies). I think it's undesirable that one repo plays both roles, and it just happens to go well if you don't have any build process (i.e. you maintain your "lib" directory by hand).

2 slightly different manifest files will be produced depending on whether you are using an object or an array of tokens?

Depending on the actual manifestContents, the result could be way different. Comments wouldn't be included, and any custom ordering of sections would get lost. That kind of stuff. I like the idea that it's possible to parse, render, parse, render the same data with practically no loss of information. That's what your tokenizer and new render logic enables, and I just want to have that available with the least amount of "ritual". I like the elegance of saying render(parse(manifestContents)). It looks better to me than render(parse(manifestContents).tokens).

What I was hoping to avoid is having to pass in data to the render function that I know it will never use, but it's just required to be there.

I didn't say I wanted to remove the option of passing a tokens array as the argument to render. That's unambiguous, and makes the API simpler in some cases, so a good feature.

[mreinstein]

I just see a world of difference between a source code repository (for logging changes in source code) and a package repository (for downloading dependencies).

I agree, but npm doesn't make the distinction; github can also host a package. Unfortunately, the reality is there are a number of times where I can't use the version in npm, because it doesn't have the features I need. In my case, I have the github based packages only until changes I need make it upstream, but sometimes that takes months, and sometimes it doesn't happen at all. If npm didn't support that capability, I'd have to clutter the package repository with more junk, and everyone loses. : /

I like the elegance of saying render(parse(manifestContents)). It looks better to me than render(parse(manifestContents).tokens).

I also agree on this point too. I suspect that's not the likely workflow, because if I'm going through the trouble of getting a tokenized version of manifest, I'm almost certainly going to be doing some modifications to it. I suspect this may be the most common usage of the tokens:

tokens = parse(manifestContents).tokens

# do stuff to tokens here, manipulating them in some way

manifest = render tokens

# write manifest to disk here?

[myrne]

I suspect that's not the likely workflow, because if I'm going through the trouble of getting a tokenized version of manifest, I'm almost certainly going to be doing some modifications to it.

Agreed. The one thing I had in mind is using it for testing purposes, that is, make parse-appcache-manifest a dev dependency of render-appcache-manifest, then test if a parse, render cycle results in the same output as an already rendered manifest, i.e. assert.equal render(parse(rendered))), rendered. I see it as a kind of "mathematical identity" (not sure that's the right word, because I suck at math).

Do you think adding this feature would be bad in any way?

[mreinstein]

Do you think adding this feature would be bad in any way? ... I see it as a kind of "mathematical identity"

I think the term you are referring to is invariance; where an object will remain unchanged after a certain set of transforms are applied to it (in our case, parse, followed by render)

The parser is not invariant, because it will apply some normalizations to the rendered document as a form of cleanup. For example if your header begins with:

CACHE MANIFEST awesome sauce, baby!

This is technically a valid magic header, but the awesome sauce, baby! won't be preserved since it's discarded by the parser.

Similarly if you have a comment in your manifest that looks like this:

#       ohhh snap son

after tokenizing and re-rendering it will be normalized to:

# ohhh snap son

[myrne]

The parser is not invariant, because it will apply some normalizations to the rendered document as a form of cleanup.

I know, that's why in my example I was careful to name the variable rendered. :)

This is what I had in mind:

rendered = render
   cache: []
   network: []
   fallback: []
assert.equal render(parse(rendered)), rendered

But also

parsed = parse(arbitraryManifestContents)
rendered = render(parsed)
assert.equal render(parse(rendered)), rendered

In short, if the manifest contents are generated by the render function, then I think the "invariance" should hold.

It actually reminds me of what I did for normalize-package-data module. There I have something that comes down to:

assert.equal normalize(normalized), normalized 

Normalizing already normalized data should result in same data.

In this case, combining parse and render could be considered normalization:

normalizedManifest = render(parse(someManifest))

[myrne]

I just merged this PR. I will add a rudimentary test myself.

[myrne]

Huh, I do see tests. So I did look over them in the commit log. Some miscommunication must have happened!

[myrne]

I see an interesting TODO, namely to group modes together in output. Why use tokens as input then? If the original manifest structure doesn't get preserved, then I think my own rendering code could be enough, or nearly so. Maybe that code just needs a few tweaks to allow some more elaborate commenting? It currently supports an array of comment lines, which are written at the bottom of the manifest.

[mreinstein]

Huh, I do see tests

yeah I didn't write super great render tests, but I did throw a very basic example in there of comparing a rendered token array against an expected manifest output. Open for improvement suggestions.

This is what I had in mind:

IMO I'd rather not have to pass in 2 data structures representing the same thing, with one copy being ignored. But you are the package maintainer, and I don't think it's worth forking for what amounts to small API differences, would much rather work together. :) If you want to change the API to use a tokens sub object if it's present, I don't have a big problem with it.

namely to group modes together in output. Why use tokens as input then?

Grouping tokens into similar sections is one possible transform of many. perhaps for different applications, one may want to preserve comments, newlines, etc, or group using slightly different rules. I haven't thought it through very carefully, but I could see it being added as a potential option in the future. Or maybe it goes into a separate npm module. Not sure, open for suggestions.

[myrne]

Grouping tokens into similar sections is one possible transform of many. perhaps for different applications, one may want to preserve comments, newlines, etc, or group using slightly different rules.

I just don't see those applications. I have very simple needs, as demonstrated in the original functionality of parse and render functions. I leave it up to you!

Having separate module(s) for doing the transformations would make sense.

render someTransform parse(manifestContents).tokens

or so.

[myrne]

Note, I published this on npm as 0.3.0. Listed you as author as well.
I think you've been an exemplary contributor. :)

[mreinstein]

I just don't see those applications. I have very simple needs, as demonstrated in the original functionality of parse and render functions.

I don't have a need for it yet either, but I thought of it in the middle of development and wanted to record it somewhere, hence the comment. If someone else comes along and decides they need it, or if I do later we'll deal with it then. :)

I think you've been an exemplary contributor.

Thanks!